Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Fix isSet flag when setting a link to a not-yet created data #5081

Merged
merged 4 commits into from
Nov 15, 2024
Merged

[Core] Fix isSet flag when setting a link to a not-yet created data #5081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
509e9d9
set isset flag even if the parent is null
fredroy Oct 23, 2024
840bdb0
add tests
fredroy Oct 23, 2024
01a7922
add test with a bogus link path
fredroy Nov 13, 2024
587110c
Merge branch 'master' into fix_isset_data
bakpaul Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Sofa/framework/Core/src/sofa/core/objectmodel/BaseData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,14 @@ bool BaseData::setParent(BaseData* parent, const std::string& path)
addInput(parent);
BaseData::setDirtyValue();
m_counter++;
m_isSet = true;
}else if (!path.empty())
}
// the referenced parent data has not been created yet but a path has been given
else if (!path.empty())
{
parentData.setPath(path);
}

m_isSet = true;

return true;
}
Expand Down
52 changes: 52 additions & 0 deletions Sofa/framework/SimpleApi/test/SimpleApi_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,55 @@ TEST_F(SimpleApi_test, createParamString )
{
ASSERT_TRUE( testParamString() );
}

TEST(SimpleApi_test_solo, testIsSetWithDataLink)
{
const Simulation::SPtr simu = createSimulation("DAG");
const Node::SPtr root = createRootNode(simu, "root");

// test not set
const auto obj1 = createObject(root, "DefaultAnimationLoop", {
{"name", "loop1"}
});
auto* objdata1 = obj1->findData("printLog");
ASSERT_FALSE(objdata1->isSet());

// test set
const auto obj2 = createObject(root, "DefaultAnimationLoop", {
{"name", "loop2"},
{"printLog", "false"}
});
auto* objdata2 = obj2->findData("printLog");
ASSERT_TRUE(objdata2->isSet());

// test set through a link of a already created object
const auto obj3 = createObject(root, "DefaultAnimationLoop", {
{"name", "loop3"},
{"printLog", "@/loop2.printLog"}
});
auto* objdata3 = obj3->findData("printLog");
ASSERT_TRUE(objdata3->isSet());

// test set through a link of a not yet created object (deferred linking)
const auto obj4 = createObject(root, "DefaultAnimationLoop", {
{"name", "loop4"},
{"printLog", "@/loop5.printLog"}
});
const auto obj5 = createObject(root, "DefaultAnimationLoop", {
{"name", "loop5"},
{"printLog", "true"}
});
auto* objdata4 = obj4->findData("printLog");
ASSERT_TRUE(objdata4->isSet());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to add a test with a wrong link ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to add a test with a wrong link ?

done.

Actually this remark leads to interesting consequences:

  • there is no message/warning whatsoever if the link is wrong (i.e point to the non-existing data). This is understandable if setting a link a posteriori is a feature;
  • more philosophical : should it be still isSet() if it is wrong ? but the link itself (no the pointed data) is set after all;
  • the only way to know the link was wrong is to test if getParent() is null... which would mean ideally that it should be tested every time...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this is the result of a not very well defined/consistent behavior.
Actually I'm very unconvident on the general use of links that are searched "a posteriori" and wonder if we shouldn't remove and replace that by a different (and explicit mechanism).


// test link with a wrong path (or non existent parent)
const auto obj6 = createObject(root, "DefaultAnimationLoop", {
{"name", "loop6"},
{"printLog", "@/loop7.printLog"}
});

auto* objdata6 = obj6->findData("printLog");
ASSERT_TRUE(objdata6->isSet());
ASSERT_EQ(objdata6->getParent(), nullptr);

}
Loading