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

Replace sphere and cylinder with smooth meshes #1463

Conversation

wxmerkt
Copy link
Contributor

@wxmerkt wxmerkt commented Jan 10, 2020

Fixes #1461 by replacing the existing meshes with meshes that have correct normals and flat shading turned on.

Changes

  • Sphere: Replaced mesh with correct normals as well as shading (may have more vertices, unsure)
  • Cylinder: Replaced with mesh with correct normals and shading turned on
  • Cone: Replaced with mesh with correct normals and shading turned on

I first used Blender and the blender2ogre exporter to work export a new model of a sphere. However, due to bugs with the export this did not work with the cylinder which is why the cylinder and cone were created from scratch in Autodesk 3DS Max.

Sphere

Before

rviz_screenshot_2020_01_09-17_46_02

After

rviz_screenshot_2020_01_09-17_49_14

Cylinder

Before

20200110-cylinder-old

After

20200110-cylinder-new

@wjwwood
Copy link
Member

wjwwood commented Jan 11, 2020

I have no idea what drawbacks this might have, but I imagine it's fine. My main concern would be performance implications, but I have no evidence or experience to say one way or the other.

@rhaschke
Copy link
Contributor

@wxmerkt, could you measure the time to render NxNx3 spheres/cylinders/cones (with N being a reasonable number e.g. 100 or 1000) to learn about the slowdown on Mesa / nvidia setups?

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 11, 2020

I did a test with 18 spheres with 100 Hz update and I didn't see a difference in GPU load as measured by nvidia-smi (given, on a top of the line machine with i9-9900K and RTX2080). I assume for measure time to render we'd need to print from the rviz c++ source as it likely won't have a measurable effect on FPS (in fact, I don't see it)?

I just ran a test with 100x100x3 spheres (so that is 30k Markers - not a sphere list!):

  • Old spheres: 10 fps (limiting factor CPU, one core at 100%)
  • New spheres: 10 fps (limiting factor CPU, one core at 100%)

image

In practice, I think robot models are in general way more complex than the basic shape introduced here. As an alternative, I am happy to use the PrefabTypes from OGRE - but they trigger the issues with the scaling and material handling that I addressed in the other branch (the one linked in #1462).

@rhaschke
Copy link
Contributor

Thanks, looks good. To reduce CPU load, it should be fine to use a sphere list.
Eventually we want to measure the rendering impact...
Doesn't work for cylinders and cones though.

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 11, 2020

Eventually we want to measure the rendering impact...

Okay - is the PR good to go ahead or is there something else I should take a look at? :-)

@rhaschke
Copy link
Contributor

Sorry that I was not clear enough: I think the PR is ready to be accepted.
But I would like to ask you to repeat your performance measurement for a sphere list of 100x100 spheres - in the hope that the CPU load to handle them is much less and we can really compare rendering performance.

@rhaschke
Copy link
Contributor

One more thing: Could you please extend the main post of this PR and describe what you have actually changed in the .mesh files? I guess the original files didn't define surface normals and you just added correct ones, right? In this case (i.e. you didn't add more vertices), there shouldn't be any performance difference, because normal interpolation should have been in place before as well - trivially resulting in the same normal per surface.

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 12, 2020

Thank you. I have updated the first post. I think the cone and cylinder have the same number of vertices; the sphere may have more. I initially modelled it in Blender but this showed to cause issues with the correct export of the normals. As a result a colleague created new files with Autodesk 3DS form scratch. I did not compare the number of vertices for all of them.

@rhaschke
Copy link
Contributor

I did not compare the number of vertices for all of them.

This would be important to judge impact on performance.

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 12, 2020

Here is the breakdown:

Old New
Cone 90 vertices, 30 faces 120 vertices, 40 faces
Cube 36 vertices, 12 faces unchanged
Cylinder 180 vertices, 60 faces 120 vertices, 40 faces
Sphere 672 vertices, 224 faces 482 vertices, 960 faces

@rhaschke
Copy link
Contributor

Thanks for this detailed breakdown. I think that cylinder and cone are fine.
However, I don't understand why the number of faces in spheres grows by a factor of 4, while the number of vertices even decreases. Maybe you can redo the sphere with blender again?
I remember as well an issue with blender's normal export. But, I was able to fix this using meshlab to import, fix, and re-export .stl files in my case. Not sure, meshlab can handle .mesh files.

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 13, 2020

The difference in vertices is that the original meshes did not reuse vertices for faces. The new ones do.

I've remodeled the sphere from scratch in Blender just now. The default sphere has 482 vertices and 512 original faces, with 960 triangles. I switched to using a SPHERE_LIST and tried the above outlined test for N=1000 (3M spheres). Both the sphere currently in master and the one in this PR reached 162fps with 3M spheres on my workstation, i.e., no difference.

@rhaschke rhaschke merged commit 2cf0b24 into ros-visualization:melodic-devel Jan 13, 2020
@rhaschke
Copy link
Contributor

Thanks for your effort.

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 13, 2020

Thank you for the feedback and time in reviewing this PR. Is there a plan for a new release/something else waiting for a patch release?

@rhaschke
Copy link
Contributor

I just prepared a new release by the end of the last year. Don't expect a new release anytime soon.

@wxmerkt
Copy link
Contributor Author

wxmerkt commented Jan 13, 2020

Okay, thanks for the heads-up. If I could help out with it, happy to do so

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.

Smooth rendering of spheres
3 participants