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 for material_name = "" bug #1462

Conversation

wxmerkt
Copy link
Contributor

@wxmerkt wxmerkt commented Jan 9, 2020

Found this while debugging mesh loading - if the material name is empty, a segfault happened. This also defaults to the default material.

@rhaschke
Copy link
Contributor

rhaschke commented Jan 9, 2020

The material shouldn't be empty actually, so I assume there is a deeper bug.
Can you provide a URDF that caused this issue?

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 10, 2020

Hi @rhaschke,
The way I triggered this bug was by switching to a PreFabType entity in createEntity. While I am now pursuing a new fix to #1461 by replacing the meshes, my first attempt is in this branch:

melodic-devel...wxmerkt:wxm-smooth-spheres

To trigger the bug, I loaded a URDF that had a mesh as the visual and a sphere as the collision element. Without this check for empty(), we would trigger a segmentation fault using the above branch with the Fix commit reverted when it tried to setMaterial on the submesh.

@rhaschke
Copy link
Contributor

If I understand you correctly, the issue is only observed in your branch to solve #1461, but not in the current master. Hence, I think your branch doesn't set the materials correctly.
If you follow another solution now (modifying sphere.mesh), I think this PR becomes obsolete.

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 10, 2020

I am now following another solution, so this won't be triggered. However, with material = "", the current master results in a segfault. Wouldn't be defaulting to the default material instead of a broken pointer resulting in a segmentation fault be a safer way to handle things?

@rhaschke
Copy link
Contributor

The segfault is an indicator that something is wrong with the material handling.
Catching this error will just hide the actual problem.

@rhaschke rhaschke closed this Jan 10, 2020
@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 10, 2020

Okay, I understand your reasoning. I still believe that if we can trigger this by using OGRE native features (PrefabType), this may happen in user plug-ins or with malformed meshes/OGRE meshes. A segmentation fault here is an indicator of a pointer being broken - and the other way to fix it could be to catch that or check it.

@rhaschke
Copy link
Contributor

As you said: A segfault indicates a malformed plugin (which doesn't comply to the rules of rviz)
and my reasoning is that this bug should be fixed there then.

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 10, 2020

I understand. I tend to catch/avoid/fix segfaults and present a helpful message/error/warning (which this PR did not do - but may consider a fix along those lines in the future). If a plugin can trigger undefined/uncaught behaviour, that's usually bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants