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

Visualize collisions #531

Merged
merged 17 commits into from
Jan 28, 2021
Merged

Visualize collisions #531

merged 17 commits into from
Jan 28, 2021

Conversation

jennuine
Copy link
Contributor

@jennuine jennuine commented Jan 5, 2021

Closes #105

Ability to visualize collision (basic shapes or meshes) similar to Gazebo-classic, which are displayed in orange.

Collision visualization can be toggled from (in the scene or in the entity tree):

  • A model's right-click context menu, which toggle all collisions in the model
  • A link's right-click context menu, which toggles all collisions in the link

Collision visuals can be created for basic shapes and meshes but depending on the render order when a collision (visual) is coplaner with the shape/mesh visual there are times when the collision visual can not be seen. This should be resolved in Edifice from this PR gazebosim/gz-rendering#188

Here's an example:
Peek 2021-01-05 17-44

@jennuine jennuine requested a review from iche033 January 5, 2021 23:05
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 5, 2021
@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #531 (9f653db) into ign-gazebo3 (347fd51) will decrease coverage by 0.02%.
The diff coverage is 20.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #531      +/-   ##
===============================================
- Coverage        77.73%   77.71%   -0.03%     
===============================================
  Files              208      208              
  Lines            11590    11595       +5     
===============================================
+ Hits              9010     9011       +1     
- Misses            2580     2584       +4     
Impacted Files Coverage Δ
src/gui/plugins/modules/EntityContextMenu.cc 19.23% <20.00%> (+0.08%) ⬆️

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 347fd51...9f653db. Read the comment docs.

@chapulina chapulina added close the gap Features from Gazebo-classic GUI Gazebo's graphical interface (not pure Ignition GUI) rendering Involves Ignition Rendering labels Jan 6, 2021
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

Works well for me. Did a first review pass and left some comments.

include/ignition/gazebo/rendering/SceneManager.hh Outdated Show resolved Hide resolved
src/gui/plugins/modules/EntityContextMenu.cc Outdated Show resolved Hide resolved
src/gui/plugins/modules/EntityContextMenu.qml Show resolved Hide resolved
src/gui/plugins/scene3d/Scene3D.cc Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
// create new collision visuals
{
if (!newCollisionLinks.empty())
DeselectAllEntities();
Copy link
Contributor

Choose a reason for hiding this comment

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

is this done so that the object does not get highlighted with bright white emissive material?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done to avoid encountering this error after a new collision was created and clicking elsewhere (deselecting an entity) https:/ignitionrobotics/ign-gazebo/blob/4a1cba6d2e32e493c5dc0f7233db01b406681adf/src/rendering/RenderUtil.cc#L1427-L1428

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried commenting the above two lines out to see what's causing this error msg appear but I couldn't reproduce it by selecting the model and toggling View > Collision. Can you list the steps to reproduce this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Start up Ignition with a sample world
  2. Select a model in Scene3D so that it is highlighted
  3. Then right click the model, View > Collisions
  4. Then click anywhere else in the scene and the error should appear

The error only appears once, after the collision visual has been created. If you toggle off the collision then run through the above steps again then the error doesn't appear again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks I was able to reproduce the problem now. I think the issue is that It was trying to update the emissive property of a new geometry (collision visual's geom) that it does not previously know about. The current code works around this problem by deselecting all entities before creating the new collision visual. But there would now a mismatch between what's selected in scene 3d and the entity tree (and other plugins) for example. It's more of a usability issue now. I would recommend seeing if there's another way around this without deselecting all entities, e.g. updating the logic in the LowlightNode function or simply removing that error msg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the issue by adding the collision visual's geometry emissive property to the originalEmissive map after creation of the collision visual: df7f009

[&](const Entity &_entity, const components::Collision *)->bool
{
this->removeEntities[_entity] = _info.iterations;
this->viewingCollisions.erase(_entity);
Copy link
Contributor

Choose a reason for hiding this comment

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

do you also need to update entityCollisions and linkToCollisionEntities, and probably also modelToLinkEntities when links are removed? hmm there's a bit of maintenance that need to be done to keep these maps update-to-date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8522398. Since only a model or a light can be removed from the gui (not a link or a collision), I didn't have to iterate through the maps to keep them updated.

sdf::Visual visual;
visual.SetGeom(*collision.Geom());
visual.SetMaterial(material);
this->dataPtr->sceneManager.CreateVisual(colEntity, visual, link);
Copy link
Contributor

Choose a reason for hiding this comment

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

The code here for creating collision visuals would be a good addition to the SceneManager class, i.e. a new CreateCollision function

and we just have to call rendering::VisualPtr colVisual = this->dataPtr->sceneManager.CreateCollision(colEntity, collision, 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.

@jennuine jennuine self-assigned this Jan 14, 2021
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

New changes look good. I couldn't visualize collision for the ground plane. Does it work for you?

// create new collision visuals
{
if (!newCollisionLinks.empty())
DeselectAllEntities();
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried commenting the above two lines out to see what's causing this error msg appear but I couldn't reproduce it by selecting the model and toggling View > Collision. Can you list the steps to reproduce this?

@jennuine
Copy link
Contributor Author

jennuine commented Jan 15, 2021

New changes look good. I couldn't visualize collision for the ground plane. Does it work for you?

No, but it's because it's missing the <size> element in the geometry of the SDF. So for example, in the diff_drive.sdf example the ground_plane has https:/ignitionrobotics/ign-gazebo/blob/b432081c6b9f1926285f448c5ff3a24a528cfcba/examples/worlds/diff_drive.sdf#L55-L61

and if <size> is added:

 <collision name="collision"> 
   <geometry> 
     <plane> 
       <normal>0 0 1</normal> 
       <size>100 100</size>
     </plane> 
   </geometry> 
 </collision> 

The collision for the ground plane is viewable. It's required spec of SDFormat, should I add the <size> element for all the examples?

@iche033
Copy link
Contributor

iche033 commented Jan 16, 2021

No, but it's because it's missing the element in the geometry of the SDF.

that's interesting because I thought if a required field is missing, sdf parser would complain. Even if it went through the parser, the default value should 1x1m which is not the behavior we're seeing. So I think we should add <size> to <plane> in the ign-gazebo worlds. Can you do that in a separate PR?

@jennuine
Copy link
Contributor Author

Can you do that in a separate PR?

Yes, I'm assuming branched from and merged to ign-gazebo3?

@iche033
Copy link
Contributor

iche033 commented Jan 19, 2021

Yes, I'm assuming branched from and merged to ign-gazebo3?

yes, thanks!

@jennuine jennuine mentioned this pull request Jan 20, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I haven't finished reviewing everything yet, but I think there's a problem with the collision poses:

Peek 2021-01-20 18-58

@@ -21,6 +21,7 @@
#include <string>
#include <vector>

#include <sdf/Collision.hh>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this include needed by the header? Can it be moved to the cc file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, done dbf7a0c

src/gui/plugins/modules/EntityContextMenu.qml Show resolved Hide resolved
src/gui/plugins/modules/EntityContextMenu.qml Show resolved Hide resolved
Signed-off-by: Jenn Nguyen <[email protected]>
@jennuine
Copy link
Contributor Author

I think there's a problem with the collision poses

Oops, thank you for catching that. It is fixed in dbf7a0c

@nkoenig
Copy link
Contributor

nkoenig commented Jan 28, 2021

Works for me.

Copy link
Contributor

@chapulina chapulina 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel close the gap Features from Gazebo-classic GUI Gazebo's graphical interface (not pure Ignition GUI) rendering Involves Ignition Rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants