Revert "Support calculating normals for indexed meshes" (#12716) and add support for calculating smooth normals

[adithramachandran]

Objective

• Refactor the changes merged in Support calculating normals for indexed meshes #11654 to compute flat normals for indexed meshes instead of smooth normals.
• Fixes Revert "Support calculating normals for indexed meshes" #12716

Solution

• Partially revert the changes in Support calculating normals for indexed meshes #11654 to compute flat normals for both indexed and unindexed meshes in compute_flat_normals
• Create a new method, compute_smooth_normals, that computes smooth normals for indexed meshes
• Create a new method, compute_normals, that computes smooth normals for indexed meshes and flat normals for unindexed meshes by default. Use this new method instead of compute_flat_normals.

Testing

• Run the example with and without the changes to ensure that the results are identical.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

@ManevilleF could I get your review here?

[IceSentry]

Like I said in my comment I'd prefer if the gltf loader wasn't changed and always use compute_flat_normals.

Right now it would lead to some meshes having flat normals and some smooth normals and it won't be obvious why unless you know about this.

Other than that, LGTM, thanks for picking this up.

[ManevilleF]

Some thoughts, there are some comments unrelated to this PR but to the one it's reverting.

• I don't know if a unified compute_normals that makes a choice between smooth and flat is useful
• The unindexed version of compute_flat_normals makes the assumption we duplicated vertices, and this is no longer mentioned, and hard to validate

[IceSentry]

I don't know if a unified compute_normals that makes a choice between smooth and flat is useful

Yeah, I think I originally mentioned a function named like that because people were trying to add smooth normals to a function called compute_flat_normals so it made more sense to have a function that didn't explicitly mention flat/smooth in the name. But if we have both functions, just let users pick I guess. Although, there might still be an argument to have something like that for users that don't care and just want something to be computed.

[IceSentry]

The unindexed version of compute_flat_normals makes the assumption we duplicated vertices, and this is no longer mentioned, and hard to validate

Right, good catch. I think the solution I would prefer is to keep the previous behaviour of panicking and suggesting users to either use compute_smooth_normals or duplicate_vertices/compute_flat_normals.

[IceSentry]

Actually, if we go back to the 0.13 behaviour of panicking then a compute_normals unified function makes more sense because it will try to pick something without panicking. The doc should definitely explain this though.

[IceSentry]

@adithramachandran here's the code of the currently released version https://github.com/bevyengine/bevy/blob/latest/crates/bevy_render/src/mesh/mesh/mod.rs#L534

I think compute_flat_normals should be identical after your PR. Except, pointing users in the assert message to consider compute_smooth_normals when indices are used, but keep the duplicate_vertices suggestion that is already present

[robtfm]

I’m not sure I fully understood the discussion but I would prefer the gltf loader not to panic in circumstances where it’s possible to find a viable strategy (thinking about user generated content).

Would the proposed changes mean indexed meshes with no normals would panic? If so are we really confident that panicking is the best answer?

If so I guess we can catch the panic to avoid crashing (I have a pr for that) or perhaps add a loader setting (which could be a separate pr as well). But I just want to make sure that’s really necessary.

[IceSentry]

I’m not sure I fully understood the discussion but I would prefer the gltf loader not to panic in circumstances where it’s possible to find a viable strategy (thinking about user generated content).

For me the main issue here is to make sure compute_flat_normals actually computes flat normals always. The currently released version does panic, so while I would also like an approach that doesn't panic I think it's more important to resolve the case of compute_flat_normals not computing flat normals. We can use compute_normals that picks flat/smooth normals based on the presence of indices, that part I don't mind much.

[ManevilleF]

For me the main issue here is to make sure compute_flat_normals actually computes flat normals always. The currently released version does panic, so while I would also like an approach that doesn't panic I think it's more important to resolve the case of compute_flat_normals not computing flat normals. We can use compute_normals that picks flat/smooth normals based on the presence of indices, that part I don't mind much.

Makes sense, this means that this PR approach to allows both:

• compute_normals enables to have normals in a non panicking way, no matter the presence of indices
• compute_flat_normals allows both indexed and non indexed meshes, which is an improvement as we don't necessarily need to duplicate vertices for flat normals

We could add a doc comment explaining that a certain layout of vertices are expected for non indexed geometry, similar to the output of duplicate_vertices

[adithramachandran]

Thanks for the comments! I'm trying to synthesize the conversation here into a set of changes that I need to make to unblock the merge:

• Implement the hygiene changes suggested by @ManevilleF (chunk_exact instead of the roundabout iteration using pointers, unifying match branches, etc.)
• In compute_flat_normals:
  • Add a doc comment explaining the constraints around unindexed meshes
  • In the assert message, tell users to consider using compute_smooth_normals with indexed meshes (keep the existing recommendation to call duplicate_vertices
  • In the unindexed branch, assert that we have duplicated vertices ("keep the previous behaviour of panicking")

Things that won't be changed:

• compute_normals, since it allows users to compute normals regardless of whether the mesh is indexed
• compute_flat_normals should continue to handle both indexed and non-indexed meshes, which allows users to not necessarily have to call duplicate_vertices for indexed meshes.

Does that sound complete?

[IceSentry]

Does that sound complete?

Yes, that seems good to me.

You didn't explicitly mentioned it but also you should definitely use the Vec3 thing @ManevilleF mentioned.

[adithramachandran]

The two points about updating the assert in the unindexed branch of compute_flat_normals don't seem to hold true, since that assert doesn't exist anymore (if indices are defined, we use them). I've tried to encapsulate the information about the layout of vertices in an unindexed mesh in the doc comment instead.

I also noticed that the Mesh::with_computed[_/_smooth_/_flat_]normals need to be created/updated to reflect the underlying implementations, so I've included those in the updated commit as well.

[IceSentry]

The two points about updating the assert in the unindexed branch of compute_flat_normals don't seem to hold true, since that assert doesn't exist anymore (if indices are defined, we use them). I've tried to encapsulate the information about the layout of vertices in an unindexed mesh in the doc comment instead.

Kinda, for me the issue is that the indexed path just assumes that duplicate vertices was called and if it wasn't called then it just generates wrong normals without warnings. That's why I mentioned just going back to the previous behaviour of not trying to do it for indexed meshes, but I guess it doesn't matter too much since it is documented in the doc comment.

I don't want to block this PR though, so I'll just approve it anyway and we'll figure something out in the future if it becomes an issue.

[IceSentry]

Everything LGTM now.