Fix flattening logic for nested model names (sdf6) #597
Conversation
j-rivero
changed the title
Signed-off-by: Jose Luis Rivero <[email protected]>
When nested models use names composed by several elements (i.e: my_model::link) these were not converted by the flattening logic inside parser.cc. The change makes the logic to work with composed names. Added a test to check that this is indeed working as expected. Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
j-rivero
force-pushed
the
nested_flattening_naming
branch
from
5e2362a to
338d966
Compare
There was a problem hiding this comment.
I think we only need to handle case B.
Case A handles a name that starts with :: and I don't think that's valid in SDFormat.
Case C handles a name where the old name is preceded by some other name, some_name::old_name, and it renames it to some_name::new_prefix::old_name. But for some_name::old_name to exist, there must be a nested model called some_name and a child element of that nested model called old_name:
<model name='included_model'>
<link name='L1'/>
<model name='some_name'>
<link name='old_name'/>
</model>
<joint name='J1' type='fixed'>
<parent>some_name::old_name</parent>
<child>L1</child>
</joint>
</model>But if this was the case, case B would have already renamed some_name to new_prefix::some_name and so we would want <parent>some_name::old_name</parent> to be renamed to <parent>new_prefix::some_name::old_name</parent> not<parent>some_name::new_prefix::old_name</parent>
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
I think you are right, changed in 4b97716 |
src/parser.cc
Outdated
| std::string::size_type found = nameWithNestedPrefix.find(oldName); | ||
| if (found != std::string::npos) | ||
| { | ||
| std::string nestedPrefix = nameWithNestedPrefix; | ||
| nestedPrefix.erase(found, oldName.length()); | ||
| replace_all(str, nestedPrefix + nestedPrefix, nestedPrefix); |
There was a problem hiding this comment.
Is this still needed after removing some of the replace_all cases?
There was a problem hiding this comment.
Umm I think you are right, this is not needed anymore after reducing the cases to just one. Removed 0f7ccf8
| { | ||
| std::string elemName = elem->Get<std::string>("name"); | ||
| std::string newName = modelName + "::" + elemName; | ||
| replace[elemName] = newName; | ||
| } | ||
|
|
||
| if (elem->GetName() == "link") |
There was a problem hiding this comment.
Quick note that this will be in conflict with #596, but we'll definitely need the line from there.
There was a problem hiding this comment.
+1 going to merge both, I'll keep an eye on it, tests should fail we miss it.
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
|
should this be merged forward to sdf9? |
The IncludeFlatteningNames test added in #597 is no longer relevant because nested model names are no longer flattened. The model used by the test is removed as well since it is not used elsewhere. Signed-off-by: Steve Peters <[email protected]>
|
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1 |
|
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1 |
🦟 Bug fix
Fixes #574
Summary
I think that the current flattening logic deal with names using
<,>,\or'as separators. That leaves out some cases were the user use composed names (component::element) since the separator here is::. I added some logic to deal with use cases were element is present in different situations with other components of the name:::component1::element::component2>element::component::component::element<Checklist
Note to maintainers: Remember to use Squash-Merge