Compute better smooth normals for cheaper, maybe #16050
Conversation
Add `face_area_normal()` function. Change `compute_smooth_normals()` from computing normal from adjacent faces with equal weight to use the area of the faces as a weight. One test added which shows the bigger triangle having an effect on the normal.
tychedelia
added
C-Feature
|
I implemented the
In the top left, "low poly" shows the flat normals, which are unchanged by this patch. In the top right, "explicit normals" are from the heart builder; they should be considered best. Bottom left is "unweighted"; this is what It is a little difficult to see any changes side to side, so I made a gif where the changes present do tend to pop out more.
For the case above, I'd judge the "weighted" image to match the "explicit normals" image slightly better than the "unweighted." The visual differences have been relatively slight. Because most of the differences are slight though, I would not be inclined to include |
crates/bevy_mesh/src/vertex.rs
Outdated
| (b - a).cross(c - a).into() | ||
| } | ||
|
|
||
| pub(crate) fn face_normal(a: [f32; 3], b: [f32; 3], c: [f32; 3]) -> [f32; 3] { |
There was a problem hiding this comment.
You could probably reuse face_area_normal here and just call .normalize() on the output.
There was a problem hiding this comment.
In the body of face_normal()? I thought about that, but I figured it'd be an extra function call. Maybe that's C thinking though. However, I'd have to call Vec3::from() again to call normalize from face_area_normal() since we'd outputting [f32; 3].
Happy to do it if you think it's worth it.
There was a problem hiding this comment.
Oh, right, I missed that it outputs a [f32:3]. Although, I wouldn't be surprised if the compiler just optimizes all of it out. Same thing for the function call. Could add an inline annotation if it's an issue. Doesn't really matter though, it's just a few lines so duplication is fine.
There was a problem hiding this comment.
I'll add the inlines and make them pub.
There was a problem hiding this comment.
I don't know how I feel about the results being slightly different. I guess it makes sense, but I'm wondering if it could break in some edge cases. At least if it breaks it's not too hard for a user to write a custom function that fixes it.
face_normal and face_area_normal could probably be pub. It doesn't hurt anything to expose that to users.
IceSentry
added
S-Ready-For-Final-Review
# Objective Avoid a premature normalize operation and get better smooth normals for it. ## Inspiration @IceSentry suggested `face_normal()` could have its normalize removed based on [this article](https://iquilezles.org/articles/normals/) in PR bevyengine#16039. ## Solution I did not want to change `face_normal()` to return a vector that's not normalized. The name "normal" implies it'll be normalized. Instead I added the `face_area_normal()` function, whose result is not normalized. Its magnitude is equal two times the triangle's area. I've noted why this is the case in its doc comment. I changed `compute_smooth_normals()` from computing normals from adjacent faces with equal weight to use the area of the faces as a weight. This has the benefit of being cheaper computationally and hopefully produces better normals. The `compute_flat_normals()` is unchanged and still uses `face_normal()`. ## Testing One test was added which shows the bigger triangle having an effect on the normal, but the previous test that uses the same size triangles is unchanged. **WARNING:** No visual test has been done yet. No example exists that demonstrates the compute_smooth_normals(). Perhaps there's a good model to demonstrate what the differences are. I would love to have some input on this. I'd suggest @IceSentry and @stepancheg to review this PR. ## Further Considerations It's possible weighting normals by their area is not definitely better than unweighted. It's possible there may be aesthetic reasons to prefer one over the other. In such a case, we could offer two variants: weighted or unweighted. Or we could offer another function perhaps like this: `compute_smooth_normals_with_weights(|normal, area| 1.0)` which would restore the original unweighted sum of normals. --- ## Showcase Smooth normal calculation now weights adjacent face normals by their area. ## Migration Guide
# Objective Closes #18383 ## Solution Given the 2 votes (me and @komadori ) for making angle-weighted normals the default, I went ahead and did so, moving the face-weighted implementation to the new `Mesh::compute_face_weighted_normals` method. I factored out the common code between both into `Mesh::compute_custom_smooth_normals`, which I went ahead and made public to make it easier for users to add any other weighting methods they might come up with. If any of these decisions are undesirable for any reason, please let me know and I will gladly make modifications. ## Testing & Showcase I made a demo that exaggerates the difference at [Waridley/bevy_smooth_normals_comparison](https://github.com/Waridley/bevy_smooth_normals_comparison). Screenshots included in the readme. Another way it could be demonstrated is via scaling a mesh along its normals, like for generating outline meshes with inverted faces. I'd be willing to make a demo for that as well. I also edited and renamed the `compute_smooth_normals` tests to use face-weighted normals, and added a new test for angle-weighted ones which validates that all normals of a unit cube have each component equal to `(±1 / √3) ± f32::EPSILON`. ## Migration Guide #16050 already did not mention a migration guide, and it is not even in a stable release yet. If this lands in a 0.16 RC, updating from RC1 would probably not require any changes in the vast majority of cases, anyway. If someone really needs face-weighted normals, they can switch to `.compute_face_weighted_normals()` or `.with_computed_face_weighted_normals()`. And if for some reason they really liked the old count-weighted implementation from 0.15, there is an example in the docs for `compute_custom_smooth_normals`.
5 participants
Objective
Avoid a premature normalize operation and get better smooth normals for it.
Inspiration
@IceSentry suggested
face_normal()could have its normalize removed based on this article in PR #16039.Solution
I did not want to change
face_normal()to return a vector that's not normalized. The name "normal" implies it'll be normalized. Instead I added theface_area_normal()function, whose result is not normalized. Its magnitude is equal two times the triangle's area. I've noted why this is the case in its doc comment.I changed
compute_smooth_normals()from computing normals from adjacent faces with equal weight to use the area of the faces as a weight. This has the benefit of being cheaper computationally and hopefully produces better normals.The
compute_flat_normals()is unchanged and still usesface_normal().Testing
One test was added which shows the bigger triangle having an effect on the normal, but the previous test that uses the same size triangles is unchanged.
WARNING: No visual test has been done yet. No example exists that demonstrates the compute_smooth_normals(). Perhaps there's a good model to demonstrate what the differences are. I would love to have some input on this.
I'd suggest @IceSentry and @stepancheg to review this PR.
Further Considerations
It's possible weighting normals by their area is not definitely better than unweighted. It's possible there may be aesthetic reasons to prefer one over the other. In such a case, we could offer two variants: weighted or unweighted. Or we could offer another function perhaps like this:
compute_smooth_normals_with_weights(|normal, area| 1.0)which would restore the original unweighted sum of normals.Showcase
Smooth normal calculation now weights adjacent face normals by their area.
Migration Guide