-
Notifications
You must be signed in to change notification settings - Fork 98
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.
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
Store include info in Element #509
Conversation
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet! Minor nits, and a testing update.
test/integration/nested_model.cc
Outdated
const std::string modelRootPath = sdf::filesystem::append( | ||
PROJECT_SOURCE_PATH, "test", "integration", "model"); | ||
|
||
sdf::setFindCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit Consider using non-global version? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using non-global version in 2631da1
auto *testModel2 = test2->ModelByIndex(0); | ||
ASSERT_NE(nullptr, testModel2); | ||
checkIncludeElem( | ||
testModel2->Element()->GetIncludeElement(), "test_model", "test_model"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully 1000% convinced that this preserves the information across load str -> element -> dump str.
Can you add a quick idempotent check that the include markup is preserved as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per f2f, this implies that the //include
tags would be preserved, which should not be the default libsdformat behavior.
Perhaps there should be an option (API + cmdline) to preserve includes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an issue to capture this: #522. How about we tackle that in a follow up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Did a first pass and left some minor comments. I'm curious, is includeFilename
needed anymore?
If so, should Element::ToString()
be updated since <include filename="path">
is no longer valid? (or removed)
https://github.com/osrf/sdformat/blob/69dc1542641df2586d2e8dcfb02e307a8a865fdb/src/Element.cc#L535-L536
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome!
On the topic of the ToString
function, would a good way of re-implementing it be to use the output of PrintValuesImpl
, but modify the fields depending on what is stored in includeElement
, the pose
, static
flags, etc?
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
I think this is actually how it works currently. The parser overrides all those parameters of the included model based on the contents of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Minor nit: I think one of your comments is missing "is" in the statement but feel free to disregard
Signed-off-by: Addisu Z. Taddese <[email protected]>
🎉 New feature
Closes #401
Summary
This stores the contents of
<include>
inside the Element loaded from the included file.For example, given the following SDFormat:
The
ElementPtr
associated with the model loaded from 'model_uri' willhave its
includeElement
set toThis can be used to retrieve additional information available under the tag after the entity has been loaded (eg.
<name>
,<pose>
,<plugin>
, etc.). An example use case for this is when saving a loaded world back to SDFormat in such a way that the<include>
tags are left intact.This also deprecates the functions
Element::SetInclude
andElement::GetInclude
since they were never used. Based on the code inElement::ToString
: https://github.com/osrf/sdformat/blob/54d9018599ce6605cee13d372c7dc51f0c0fcd2d/src/Element.cc#L528-L537I think these functions were added a long time ago when the syntax for including files was
<include filename="path">
, which is not valid SDFormat anymore.Checklist
sh tools/code_check.sh
)Note to maintainers: Remember to use Squash-Merge