-
Notifications
You must be signed in to change notification settings - Fork 475
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
Scene: support deletion of Heightmap Visuals #3171
Conversation
A Heightmap visual still appears in gzclient even when its corresponding Visual is removed using Scene::RemoveVisual, as reported in Issue #3159. This attempts to delete the ScenePrivate::terrain object when its corresponding Visual is passed to RemoveVisual by storing the corresponding Visual Id. It doesn't seem to work though. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Copy code from elsewhere in Scene::ProcessVisualMsg to fix removal of heightmap visual. * Try to find parent visual for heightmap instead of only using the world visual * Add heightmap visual to the visuals vector so it can be removed in RemoveVisual Signed-off-by: Steve Peters <scpeters@openrobotics.org>
The Heightmap visual loading code in Scene::ProcessVisualMsg was not using the same Visual::LoadFromMsg method as the other visuals. This changes it to be more consistent. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Refactor code in Scene::ProcessVisualMsg so that a Visual containing a heightmap uses more of the same code path as other visuals. This also allows removing duplicated code. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
it should be fixed by e497d4d |
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.
Test failures irrelevant to changes.
Thank you for fixing this as it's a functionality I need for a project. I might've spotted a bug however:
For some reason it uses the old visual representation (of the old heightmap) but the collisions work as intended (they're based on the new heightmap). |
Seems my previously described issue was caused by the cache problems with heightmaps similar to the comments in #2604. Deleting the |
@tekfr34k can you open an issue describing how to reproduce the problem? thanks! |
@scpeters I couldn't narrow down what exactly the problem is yet. The issue is very inconsistent, depending on if I start Gazebo from my launch scripts or just without any configuration at all it behaves differently. Maybe it has something to do with defining multiple model paths (even if the models are named differently in the SDF/config). Adding a model path in the "Insert" tab which contains the heightmap SDF gives an issue of not showing the model in the overview. Maybe that's something to start with... |
As noted in #3159, removing a model containing a heightmap causes its collision shape to be remove from the physics subsystem in
gzserver
, but the heightmap visual remains ingzclient
. This pull request fixes that so that height map visuals should now disappear fromgzclient
when a heightmap model is deleted. The sequence of commits is described below:Scene::RemoveVisual
to delete the terrain object when a visual matchingterrainVisualId
is removed. This didn't actually work.dataPtr::visuals
vector. This code was already present elsewhere inProcessVisualMsg
, so I duplicated it to apply to the heightmap codepath. I didn't like this code duplication, so I worked to eliminate that in the following two commits.Visual::LoadFromMsg
for consistency with the latter parts ofProcessVisualMsg
. It requires construction of an extraboost::shared_ptr
but I think the consistency is worth it.ProcessVisualMsg
so it can share the existing portions that it had duplicated. This commit has a net reduction of 25 lines.