-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Revert "Support calculating normals for indexed meshes" (#12716) and add support for calculating smooth normals #13333
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
@ManevilleF could I get your review here? |
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.
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.
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.
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
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. |
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. |
Actually, if we go back to the 0.13 behaviour of panicking then a |
@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 |
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. |
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:
We could add a doc comment explaining that a certain layout of vertices are expected for non indexed geometry, similar to the output of |
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:
Things that won't be changed:
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. |
The two points about updating the assert in the unindexed branch of I also noticed that the |
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. |
# Objective - Refactor the changes merged in #11654 to compute flat normals for indexed meshes instead of smooth normals. ## Solution - Partially revert the changes in #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.
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.
Everything LGTM now.
# Objective - some gltf files are broken since #13333 ``` thread 'IO Task Pool (2)' panicked at crates/bevy_render/src/mesh/mesh/mod.rs:581:9: `compute_flat_normals` can't work on indexed geometry. Consider calling either `Mesh::compute_smooth_normals` or `Mesh::duplicate_vertices` followed by `Mesh::compute_flat_normals`. ``` - test with example `custom_gltf_vertex_attribute` or `gltf_skinned_mesh` ## Solution - Call the wrapper function for normals that will either call `compute_flat_normals` or `compute_smooth_normals` as appropriate ## Testing - Ran the two examples mentioned above
Objective
Solution
compute_flat_normals
compute_smooth_normals
, that computes smooth normals for indexed meshescompute_normals
, that computes smooth normals for indexed meshes and flat normals for unindexed meshes by default. Use this new method instead ofcompute_flat_normals
.Testing