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.

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

Fix loading dae file with hierarchical node that does not have a name #101

Merged
merged 4 commits into from
Oct 12, 2020

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Sep 29, 2020

pull request #78 fixed loading hierarchical submeshes in ColladaLoader but we ran into an edge case that if a node in the dae file does not have a name, it would be given the name of the previous node. For example,the Polaris Ranger EV model model has the node structure below and the result is that the nested node inside node_B would be incorrectly given the name node_A :

<node name="node_A">
</node>
<node name="node_B">
    <node>
        ...
    </node>
</node>

The fix adds the logic to check that if a node does not have a name, it'll then be appended and become part of the parent mesh. It restores the original behavior before pull request #78 was merged while preserving the hierarchical submesh fix.

Signed-off-by: Ian Chen ichen@osrfoundation.org

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033 iche033 requested a review from mjcarroll as a code owner September 29, 2020 04:24
@github-actions github-actions bot added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Sep 29, 2020
@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #101 into ign-common3 will increase coverage by 0.02%.
The diff coverage is 90.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-common3     #101      +/-   ##
===============================================
+ Coverage        73.94%   73.96%   +0.02%     
===============================================
  Files               69       69              
  Lines             9406     9418      +12     
===============================================
+ Hits              6955     6966      +11     
- Misses            2451     2452       +1     
Impacted Files Coverage Δ
graphics/src/ColladaLoader.cc 84.06% <90.00%> (+0.04%) ⬆️
av/src/Util.cc 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 335aa84...b73e933. Read the comment docs.

Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Works great for me, thanks for the fix!

@mjcarroll
Copy link
Contributor

Looks like the Ubuntu build is erroring due to low disk space: https://build.osrfoundation.org/job/ignition_common-ci-pr_any-ubuntu_auto-amd64/395/console

CC: @j-rivero and @tfoote (current build farmer)

@mjcarroll
Copy link
Contributor

@iche033 Do you think you can add a dae file that exercises this behavior? It should make codecov a bit happier.

chapulina and others added 2 commits September 29, 2020 10:23
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Copy link
Contributor Author

iche033 commented Oct 2, 2020

yep added test in d958def

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

One spelling suggestion, but LGTM.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@mjcarroll mjcarroll merged commit 966a833 into ign-common3 Oct 12, 2020
@mjcarroll mjcarroll deleted the dae_node_name branch October 12, 2020 19:19
JShep1 pushed a commit that referenced this pull request Oct 14, 2020
…#101)

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
mjcarroll pushed a commit that referenced this pull request Oct 14, 2020
…#101)

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
mjcarroll pushed a commit that referenced this pull request Oct 19, 2020
…#101)

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants