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 ContactSensorData #234

Merged
merged 17 commits into from
May 5, 2021
Merged

Conversation

mcres
Copy link
Contributor

@mcres mcres commented Jul 8, 2020

Fixes issue #112

This requires #272

It currently visualizes the position of the contacts returned by the Contact sensors.
It also allows the user to change the size of the markers (radius and length) through a couple of spin boxes.

In order to show the contacts of all of the components::Collision, it appears it's not enough to create a components::ContactSensorData as stated here, because this is a GUI system.

issue_112

Signed-off-by: Martiño Crespo <marticres@gmail.com>
@mcres mcres requested review from azeey and chapulina as code owners July 8, 2020 16:39
@mcres mcres changed the title Initial fix: visualize ContactSensorData Visualize ContactSensorData Jul 8, 2020
@mabelzhang mabelzhang self-requested a review July 9, 2020 16:13
@mabelzhang mabelzhang added 🔮 dome Ignition Dome close the gap Features from Gazebo-classic labels Jul 11, 2020
@chapulina chapulina linked an issue Jul 14, 2020 that may be closed by this pull request
@mcres
Copy link
Contributor Author

mcres commented Jul 15, 2020

Work is being done on visualizing contacts for <collision> elements without Contact sensors.

Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

I tested with

$ ign gazebo visualize_contacts.sdf -r

Looks good. The contacts show up, but the text boxes don't show up correctly for me. I only see 1 digit. So initially I just see 0 and 0 for both radius and length. if I arrow to the left, I'd eventually see the decimal point 0., but I can never see the entire number. I dragged the panel to the left to expand it, but the text boxes didn't expand. Maybe they can be set to a longer length.

What are the units of the adjustable parameters? I would suggest putting that unit to the right of the text boxes.

src/gui/plugins/visualize_contacts/VisualizeContacts.cc Outdated Show resolved Hide resolved
src/gui/plugins/visualize_contacts/VisualizeContacts.hh Outdated Show resolved Hide resolved
src/systems/physics/Physics.cc Outdated Show resolved Hide resolved
src/systems/physics/Physics.cc Outdated Show resolved Hide resolved
src/gui/plugins/visualize_contacts/VisualizeContacts.cc Outdated Show resolved Hide resolved
src/gui/plugins/visualize_contacts/VisualizeContacts.qml Outdated Show resolved Hide resolved
@mcres
Copy link
Contributor Author

mcres commented Jul 21, 2020

The contacts show up, but the text boxes don't show up correctly for me. I only see 1 digit. So initially I just see 0 and 0 for both radius and length. if I arrow to the left, I'd eventually see the decimal point 0., but I can never see the entire number. I dragged the panel to the left to expand it, but the text boxes didn't expand. Maybe they can be set to a longer length.

Hmm that's weird. Does that happen to you if you load the Grid Config plugin, which also has spin boxes?

@mabelzhang
Copy link
Contributor

mabelzhang commented Jul 22, 2020

Hmm yeah actually it does in Grid Config plugin as well:
Screenshot from 2020-07-22 04-49-28

Looking at this, maybe you could center everything in the Visualize contacts panel too for consistency.

I think the spin boxes being too short is a GUI problem. It should probably be fixed elsewhere, but it should be fixed. I'm on a 4k laptop screen, and most windows and menus are automatically scaled up 2x. That may or may not be related to this problem. It's a pretty common laptop so others will run into this as well.

@chapulina Is this a known issue? I took a scan in ign-gui and didn't see a related ticket. Let me know if I should make one. Could be a good first issue.

@chapulina
Copy link
Contributor

the spin boxes being too short is a GUI problem

It's possible to set the width of IgnSpinBox directly inside VisualizeContacts.qml. I'd recommend making GridLayout be as large as it can be with anchors.fill: parent, then making each spin box take as much width as possible with Layout.fillWidth: true. Let me know how it goes!

mcres added 2 commits July 27, 2020 12:14
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
@mcres
Copy link
Contributor Author

mcres commented Jul 27, 2020

In order to visualize the contacts for objects without contact sensors, we've encountered some problems and solved them the following way:

  1. First of all, we had to 'enable' the <collision>. In order to create the ContactSensorData inside the Collision component, the ECM provided in the GuiSystem::Update method is not connected to the physics system. For that, I've created a /enable_collision and /disable_collision services, which enable or disable an individual collision component. Both services check that there's not a contact sensor connected to the <collision>. Probably some refactoring can be done there, please let me know if that's the case.
  2. While running the example world, I've realized that the contacts remained when two objects were no longer touching (for instance, when removing the cylinder from the top of the car). The reason is that the ContactSensorData field in the SerializedStepMap message was empty after moving the cylinder, which caused the EntityComponentManager::SetState not to correctly update the info.
    As a workaround, we've added some dummy data to emtpy ContactSensorData components in the physics system
    , which are skipped when visualizing the contacts.
  3. [Unresolved] There's a similar problem as no. 2 when disabling a collision: it seems that when removing the ContactSensorData in the service, the component still stays stored somewhere, but I can't find where. I've checked the physics system and the SerializedStepMap message, and they are not updating nor showing the component after removing it, respectively. However, the ECM is somehow still giving me that value. I've tried emtpying the contact value just before being removed, but that also doesn't work. @azeey any ideas on where that can be stored?

Also, I've intentionally removed some of the contact sensors from the example world for testing the plugin.

@mcres
Copy link
Contributor Author

mcres commented Jul 27, 2020

It's possible to set the width of IgnSpinBox directly inside VisualizeContacts.qml. I'd recommend making GridLayout be as large as it can be with anchors.fill: parent, then making each spin box take as much width as possible with Layout.fillWidth: true. Let me know how it goes!

@chapulina I've changed that and @mabelzhang should be able to see the spin boxes. The problem is the extra space between the different elements:

visualize_gui

I guess that's because of the anchors.fill: parent, I wonder if I could limit the expanding height somehow. The .qml file is very similar to that of the Grid Config plugin, which doesn't take up all of the space available. I've tried the Layout.minimumHeight property without results.

@chapulina
Copy link
Contributor

The problem is the extra space between the different elements:

The trick to press them together is to add an invisible "spacer" item at the bottom of the layout like this:

https://github.com/ignitionrobotics/ign-gazebo/blob/aea1accff6d8daee0358ae9025e2a9797d292844/src/gui/plugins/grid_config/GridConfig.qml#L319-L323

With fillHeight, you're telling the layout to give that item as much vertical space as possible.

@mabelzhang
Copy link
Contributor

mabelzhang commented Aug 6, 2020

@mabelzhang should be able to see the spin boxes

Confirming text boxes are automatically expanding for me now after dragging the panel to the left.

mcres added 3 commits August 20, 2020 17:12
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Signed-off-by: Martiño Crespo <marticres@gmail.com>
@chapulina chapulina changed the base branch from master to ign-gazebo4 October 3, 2020 01:30
@mabelzhang
Copy link
Contributor

Sounds to me like most things in this ticket are resolved, other than bullet 3 linked in the previous comment.

@azeey Is this something acceptable that we can create an issue for the future and merge this PR, or does that bullet need to be fixed before this goes in?

@azeey
Copy link
Contributor

azeey commented Oct 19, 2020

Sounds to me like most things in this ticket are resolved, other than bullet 3 linked in the previous comment.

@azeey Is this something acceptable that we can create an issue for the future and merge this PR, or does that bullet need to be fixed before this goes in?

Yeah, I think we can create an issue for it and merge this PR. @mcres, I haven't been able to reproduce the issue on the visualize_contacts.sdf example, do you have an example that shows this behavior?

@mcres
Copy link
Contributor Author

mcres commented Oct 28, 2020

@azeey This is the sequence:

  1. I ign gazebo -v 4 examples/worlds/visualize_contacts.sdf -r and manually check the contact visualization in the plugin
  2. In another terminal, I try to turn off the visualization of a specific collision --- in this case the ground floor and the right wheel, id 7 and 23, respectively--- by disabling their collisions: ign service -s /world/visualize_contacts/disable_collision --reqtype ignition.msgs.Entity --reptype ignition.msgs.Boolean --timeout 1000 --req 'type: 5; id: 7' and ign service -s /world/visualize_contacts/disable_collision --reqtype ignition.msgs.Entity --reptype ignition.msgs.Boolean --timeout 1000 --req 'type: 5; id: 23'

I used to encounter 2 issues:

  1. The contact just kept showing (which I'm not able to reproduce anymore, I guess that's good news)
  2. The last contact shown immediately before requesting the services keep showing, but there are no new contacts (this still happens, though very few times). In the image below, the car was moving when I turned off the right wheel and floor contact. The "free" blue sphere is the position of the right wheel when I turned the contact off and its position won't change.

issue

If this isn't reproducible, merging and opening an issue if it happens again makes sense to me!

@azeey
Copy link
Contributor

azeey commented Nov 2, 2020

I haven't been able to reproduce the two issues you mentioned. But I noticed what I think is a performance issue with Markers. When the update period is set to a shorter amount of time, the gui becomes jumpy after a little while

Peek 2020-11-02 11-41

This is what I think is going on. When an existing marker is modified, a new material is created for it. This material is then assigned to the existing marker visual.
https://github.com/ignitionrobotics/ign-gazebo/blob/1030417c25c8d282244050b2c068053c32e642ba/src/rendering/MarkerManager.cc#L329-L332

This causes the existing material for the marker to get removed. Over time, this becomes really expensive for some reason. My guess is Ogre has an ever increasing list of resource names that has to be traversed even if the actual active list of materials is kept small. Profiling it showed a hotspot in ignition::rendering::v4::OgreMarker::SetMaterial(std::shared_ptr<ignition::rendering::v4::Material>, bool) which eventually calls Ogre::ResourceManager::remove(std::string const&)

@mcres
Copy link
Contributor Author

mcres commented Nov 4, 2020

I see, I didn't notice about that.
Since the problem is in Ogre, is it possible to merge and create another issue? I think the PR is getting pretty big.

@azeey
Copy link
Contributor

azeey commented Nov 4, 2020

Since the problem is in Ogre, is it possible to merge and create another issue? I think the PR is getting pretty big.

That sounds good. Do you mind resolving the merge conflict?

mcres added 2 commits November 4, 2020 20:57
Signed-off-by: Martiño Crespo <marticres@gmail.com>
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks like tests are failing https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/4598/testReport/junit/(root)/ContactSystemTest/MultipleCollisionsAsContactSensors/:

/var/lib/jenkins/workspace/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/ign-gazebo/test/integration/contact_system.cc:118
Value of: contactMsgs.size()
  Actual: 10
Expected: 0u
Which is: 0

@chapulina chapulina self-requested a review November 30, 2020 19:54
@mcres
Copy link
Contributor Author

mcres commented Dec 1, 2020

Hmm I have no idea where this could come from and to be honest for me it would probably take more time than I have.
Is it okay if you take a look into this @azeey?

This fixes a test failure where the expected number of contacts is zero,
but we were sending dummy data to workaround an ECM synchronization
issue.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
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.

The contacts between the cylinder and the vehicle seem out of place:

cylinder_contacts

I think the error is coming from DART though, because the GUI isn't adding any offset.

msgs::Contacts contactsComp;

for (const auto &[collEntity2, contactData] : contactMap)
else
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this block needs to be inside an else, because the if above is returning. We could save some indentation by removing the else.

Alternatively, you could remove the block that's inside the if and let the SetData below set the empty contactsComp. Makes sense?

src/systems/user_commands/UserCommands.cc Outdated Show resolved Hide resolved
src/EntityComponentManager.cc Show resolved Hide resolved
src/gui/plugins/visualize_contacts/VisualizeContacts.cc Outdated Show resolved Hide resolved
@azeey
Copy link
Contributor

azeey commented Jan 14, 2021

The contacts between the cylinder and the vehicle seem out of place:

Looks like the issue is in ODE, the collision detector we are using for DART. Here's the same world in Gazebo classic.

image

chapulina added 2 commits May 3, 2021 15:43
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
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 addressed my final comments in c28fc20

This is good to go with happy CI 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
close the gap Features from Gazebo-classic 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visualize contacts
4 participants