SubMesh::RecalculateNormals improvement

[ntfshard]

🦟 Bug fix

Fixes #N/A

Summary

Previous version of RecalculateNormals member function has a O(n^2) complexity because inner loop where we are looking for normal elements. But we already know indexes of this elements. So we can get rid of inner loop.
And seems we should check indices.size() < 3u || indices.size() % 3u != 0 due to if it will be 4, main loop will start and on second iteration it will start reading out of array.
Also seems condition in the begin should check amount of indexes/vector, but not normals, due to we are resizing it later.
Maybe this function also should have a check of enum PrimitiveType of data, seems attempt to call it for POINTS and LINES is not legit (for rest 4 types maybe it's ok, not sure)

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[ntfshard]

I force-pushed commit due to previous one was not signed and I made boundary check more strict

[iche033]

looks like there is still an issue with DCO, can you try fix it again?

[iche033]

I was testing this with a capsule shape and I noticed that a seam is now visible after the changes:

Before:

After:

The RecaculateNormals function was ported from gazebo-classic so I went through it's history and found this:
gazebosim/gazebo-classic@ea54b56

From the commit msg, the loop was actually implemented on purpose to make objects appear smoother. The effect of that loop is that more normals are updated instead of limiting to 3 normals. Not efficient but produces smoother results. I'm leaning towards keeping the current code as it was intentional.

[ntfshard]

I see. Thank you for testing and clarification. The main problem with current code that it could work about a minute if polygon has quite a lot of triangles even on a modern hardware. Maybe I can try to re-implement initial behaviour with unordered_map. I guess it should be ok to exchange execution time to a memory consumption spike

[iche033]

Maybe I can try to re-implement initial behaviour with unordered_map. I guess it should be ok to exchange execution time to a memory consumption spike

yes that would be good to try, thanks

[ntfshard]

Sorry for a delay. Here is an another solution. We are grouping all points by x coordinate and check only points with a same x coordinate (and adjacent groups, due to operator== for Vector3d is actually treat equal a coordinate in a plus/minus Epsilon range.)

For a relatively small sub-meshes it works slower(on my hardware, around 8 milliseconds) than previous one. It looks like a minimal running time constant. Not sure how critical is it. If it's a showstopper, I can dispatch algorithms according to a input size, but it will make code more complex(for a future readers), that's why I came with this version first.

Also not sure what with Jenkins CI, I tried to run this binaries locally and they seems working fine
UPD: seems no problem, I'm just a stupid

And the last thing, I don't have a full gazebo setup in current environment, can rely only on unit tests (w/o visual manual check)

[ntfshard]

It feels like it also should check: || this->dataPtr->primitiveType != SubMesh::TRIANGLES but not 100% sure that it's only one primitiveType

[iche033]

+1, I think it's good to be explicit and check that it's a TRIANGLES primitive type. I think other primitive types don't quite work yet but this is just in case better support is implemented for other types in the future.

[iche033]

The new approach looks fine to me. I tested the code and the capsule renders fine without the seam. Just one minor comment on adding some doc to explain the logic.

[iche033]

+1, I think it's good to be explicit and check that it's a TRIANGLES primitive type. I think other primitive types don't quite work yet but this is just in case better support is implemented for other types in the future.

[iche033]

can you add some documentation to explain the logic in this function? I think it's to find and include all indices of vertices that are within the tolerance of the math::equal checks, so that their normals all count towards the final calculation?

[iche033]

looks good, thanks for iterating