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

Update collision visualization to support nested models #823

Merged
merged 5 commits into from
May 24, 2021

Conversation

atharva-18
Copy link
Contributor

@atharva-18 atharva-18 commented May 18, 2021

Signed-off-by: Atharva Pusalkar atharvapusalkar18@gmail.com

🦟 Bug fix

Fixes #767

Summary

Updates collision visualization to support SDFormat 1.8.
collisions_edifice

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>
@atharva-18 atharva-18 requested a review from iche033 as a code owner May 18, 2021 10:09
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label May 18, 2021
@ahcorde ahcorde requested a review from jennuine May 18, 2021 10:38
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for @jennuine review

Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this @atharva-18 and great work so far!

Unfortunately, this does not work for deeply nested models. In the gif, only 2 objects in the scene show collisions but all 5 should be shown when viewing all collisions for M1:

Peek 2021-05-18 11-07

Assuming you are using an Edifice workspace, to run this example:

export IGN_GAZEBO_RESOURCE_PATH=<path to Edifice workspace>/src/ign-gazebo/test/worlds/models
ign gazebo <path to workspace>/src/ign-gazebo/test/worlds/model_nested_include.sdf

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>
@atharva-18
Copy link
Contributor Author

@jennuine Thanks for finding that! It's fixed in ad5b54b.

@ahcorde
Copy link
Contributor

ahcorde commented May 19, 2021

@atharva-18 is this bug also affecting your wireframe PR?

@atharva-18
Copy link
Contributor Author

@ahcorde Yes, this bug is present in the wireframe visualization. I'll open a PR addressing it once this one gets approved.

@ahcorde ahcorde requested a review from jennuine May 19, 2021 08:54
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Great work! A few minor comments

@jennuine
Copy link
Contributor

Also, I think we should #include <stack> at the top of RenderUtil.cc (it's in alphabetical order)

https://github.com/ignitionrobotics/ign-gazebo/blob/ad5b54ba350f5dc2b41f3ec679e4ad6411057074/src/rendering/RenderUtil.cc#L18-L23

Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>
Signed-off-by: Atharva Pusalkar <atharvapusalkar18@gmail.com>
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

Thanks for iterating! LGTM, I believe the failing tests are flaky. @ahcorde can you confirm?

@ahcorde
Copy link
Contributor

ahcorde commented May 20, 2021

@osrf-jenkins retest this please

@ahcorde
Copy link
Contributor

ahcorde commented May 24, 2021

@osrf-jenkins retest this please

@ahcorde ahcorde merged commit fb973bd into gazebosim:ign-gazebo5 May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update visualize collisions to support 1.8 composition
3 participants