Add angle-weighted smooth normals implementation (#18383) #18552

Waridley · 2025-03-25T22:25:41Z

Objective

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. 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.

alice-i-cecile · 2025-03-25T22:41:33Z

This is too late for 0.16, so you should write the migration guide relative to #16050. Thanks for submitting this though: I think this is reasonable.

@komadori if you have the time, a review here would be much appreciated.

IceSentry

I like the general refactor. I haven't checked the math yet but everything looks right to me at first glance.

Could you try to benchmark if the new custom_normals thing doesn't affect performance too much? I'm mildly worried about the overhead of using the callback.

Waridley · 2025-03-26T02:57:25Z

Could you try to benchmark if the new custom_normals thing doesn't affect performance too much? I'm mildly worried about the overhead of using the callback.

Sure, I was kinda curious how significant the regression was from normalizing every triangle normal and computing the angles anyway.

Do you think it would be worth adding an official benchmark in benches, or should I just test it locally and report the results?

IceSentry · 2025-03-26T04:22:30Z

It doesn't really hurt I guess. I don't expect that many people to touch this code unlike ECS internals, but I see no reasons not to add it.

IceSentry · 2025-03-31T01:15:09Z

Could you add the benchmark as a separate PR so we could have a before/after? Your benchmark does compare the new method to the old one, but it doesn't measure the impact of using a callback.

Waridley · 2025-03-31T16:02:28Z

Could you add the benchmark as a separate PR so we could have a before/after? Your benchmark does compare the new method to the old one, but it doesn't measure the impact of using a callback.

Yeah, sorry, I ran out of time to do any more the other day.

FWIW when I tested it locally, there was no difference between marking compute_custom_smooth_normals with #[inline(always)] or #[inline(never)] or manually inlining it.

Interestingly, the difference between angle-weighted and area-weighted was smaller the larger the mesh I tested with, between ~5x for a 64x64 grid to ~3x with a 2048x2048. That's definitely a significant performance difference and I will mention it in the migration guide, but I personally still think it's best to default to angle-weighted since most of the time, people aren't re-computing normals in a tight loop every frame, but rather computing them once on startup or whenever a new terrain chunk loads. Still matters for load times and to avoid brief freezes.

IceSentry · 2025-03-31T16:04:46Z

Ah, okay, thanks for investigating. I wasn't expecting a big difference, but I just wanted to make sure it wouldn't make it noticeably slower.

As long as the performance is mentioned in the docs comment I'm fine with either being the default.

# Objective Benchmark current `compute_*_normals` methods to get a baseline as requested in #18552 (comment) ## Solution Since the change to the default smooth normals method will definitely cause a regression, but the previous method will remain as an option, I added two technically-redundant benchmarks but with different names: `smooth_normals` for whatever default weighting method is used, and `face_weighted_normals` to benchmark the area-weighted method regardless of what the default is. Then I'm adding `angle_weighted_normals` in #18552. I also added `flat_normals` for completeness.

Waridley · 2025-03-31T19:00:44Z

Migration guide might be a bit wordy, feel free to give feedback.

Waridley · 2025-03-31T19:18:40Z

I really need to figure out why CI is behaving differently for me locally 😅 I'll have to fix the markdown errors later today.

greeble-dev

Left some suggestions. Only the protection against degenerate meshes feels required - the rest are doc cleanups, or naming alternatives that are not required.

greeble-dev · 2025-05-01T14:38:59Z

crates/bevy_mesh/src/mesh.rs

@@ -770,6 +860,25 @@ impl Mesh {
        self
    }

+    /// Consumes the mesh and returns a mesh with calculated [`Mesh::ATTRIBUTE_NORMAL`].
+    ///
+    /// (Alternatively, you can use [`Mesh::compute_smooth_normals`] to mutate an existing mesh in-place)


Suggested change

/// (Alternatively, you can use [`Mesh::compute_smooth_normals`] to mutate an existing mesh in-place)

/// (Alternatively, you can use [`Mesh::compute_face_weighted_normals`] to mutate an existing mesh in-place)

greeble-dev · 2025-05-01T15:00:15Z

crates/bevy_mesh/src/mesh.rs

+    // FIXME: This should handle more cases since this is called as a part of gltf
+    // mesh loading where we can't really blame users for loading meshes that might
+    // not conform to the limitations here!


"this is called as a part of gltf mesh loading." - I don't think that's true?

I'm not sure. I was thinking of looking into resolving this fixme by bailing instead of panicking, maybe in a separate PR, but I figured the new error handling in 0.16 might change how it should be addressed so I decided to wait until after it released. I just assumed this comment which was in multiple places before was accurate and I stuck it on the common function all of the others now call. I'll try to check if it's still accurate.

Either way, maybe bailing and logging an error would still be better for now than panicking, and changing the signature to return a result could be discussed in the future?

Oh wow, it looks like this issue predates any smooth normal calculation at all. The gltf loader duplicates vertices and computes flat normals if the mesh is missing them.

bevy/crates/bevy_gltf/src/loader/mod.rs

Line 665 in cd67bac

mesh.compute_flat_normals();

Personally, I'd be happy to click approve if this PR keeps the existing panics and just adds new features.

greeble-dev · 2025-05-01T15:25:58Z

crates/bevy_mesh/src/mesh.rs

+            let pa = Vec3::from(positions[a]);
+            let pb = Vec3::from(positions[b]);
+            let pc = Vec3::from(positions[c]);
+            let weight_a = (pb - pa).angle_between(pc - pa);


Should consider checking if either vector is nearly zero. If I'm reading the angle_between code right then this could result in NaN/INF.

greeble-dev · 2025-05-01T15:28:59Z

crates/bevy_mesh/src/mesh.rs

+            let weight_b = (pa - pb).angle_between(pc - pb);
+            let weight_c = (pa - pc).angle_between(pb - pc);
+
+            let normal = Vec3::from(face_normal(positions[a], positions[b], positions[c]));


Similarly, face_normal can be unstable if the triangle is tiny/zero/degenerate.

greeble-dev · 2025-05-01T15:36:52Z

crates/bevy_mesh/src/mesh.rs

-    ///
-    /// FIXME: This should handle more cases since this is called as a part of gltf
-    /// mesh loading where we can't really blame users for loading meshes that might
-    /// not conform to the limitations here!
    pub fn compute_smooth_normals(&mut self) {


Personally, I'd prefer it if there was a separate compute_angle_weighted_normals method, and compute_smooth_normals just forwards to it. Having the option to be explicit feels a bit nicer. I don't feel strongly about it though.

greeble-dev · 2025-05-01T15:37:32Z

crates/bevy_mesh/src/mesh.rs

+    /// Panics if [`Mesh::ATTRIBUTE_POSITION`] is not of type `float3`.
+    /// Panics if the mesh has any other topology than [`PrimitiveTopology::TriangleList`].
+    /// Panics if the mesh does not have indices defined.
+    pub fn compute_face_weighted_normals(&mut self) {


compute_area_weighted_normals feels more intuitive to me, but I don't feel strongly about it.

Actually I think I agree, I was just maintaining the language from the previous PR that implemented this method in the first place. But part of my complaint with it was that it actually doesn't actually take a whole geometric "face" into account, but only immediately adjacent triangles.

greeble-dev · 2025-05-01T15:56:31Z

crates/bevy_mesh/src/mesh.rs

+    /// - [`Mesh::compute_smooth_normals`]
+    /// - [`Mesh::compute_face_weighted_normals`]
+    ///
+    /// An example that would weigh each connected triangle's normal equally, thus skewing normals


Suggested change

/// An example that would weigh each connected triangle's normal equally, thus skewing normals

/// An example that would weight each connected triangle's normal equally, thus skewing normals

greeble-dev

Looks good! Just one last request - the migration guide should note that face_normal and face_area_normal have been renamed.

github-project-automation bot added this to Rendering Mar 25, 2025

alice-i-cecile added this to the 0.17 milestone Mar 25, 2025

IceSentry reviewed Mar 26, 2025

View reviewed changes

Waridley force-pushed the angle_weighted_normals branch 2 times, most recently from aa4e9f5 to 6244acd Compare March 30, 2025 03:16

Waridley mentioned this pull request Mar 31, 2025

Add compute_*_normals benchmarks #18648

Merged

Waridley force-pushed the angle_weighted_normals branch from 6244acd to 870c8e4 Compare March 31, 2025 17:59

Waridley force-pushed the angle_weighted_normals branch 2 times, most recently from 5ed4b76 to 470b5bd Compare March 31, 2025 19:00

Waridley force-pushed the angle_weighted_normals branch from 470b5bd to 79a6103 Compare March 31, 2025 19:03

Waridley force-pushed the angle_weighted_normals branch from 79a6103 to a3af249 Compare April 1, 2025 00:58

greeble-dev suggested changes May 1, 2025

View reviewed changes

greeble-dev approved these changes May 2, 2025

View reviewed changes

Waridley added 4 commits May 2, 2025 18:59

Add angle-weighted smooth normals implementation (bevyengine#18383)

2c80e09

Add angle_weighted_normals benchmark

cf27f93

Update docs and add migration guide

5072015

Implement suggestions from @greeble-dev review

3ef6184

Update migration guide

27c9007

Waridley force-pushed the angle_weighted_normals branch from 787ed35 to 27c9007 Compare May 2, 2025 23:59

alice-i-cecile added the C-Feature A new feature, making something new possible label Jul 16, 2025

IceSentry approved these changes Jul 17, 2025

View reviewed changes

IceSentry added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 17, 2025

alice-i-cecile added this pull request to the merge queue Jul 17, 2025

Merged via the queue into bevyengine:main with commit 9e1e8bc Jul 17, 2025
36 checks passed

github-project-automation bot moved this to Done in Rendering Jul 17, 2025

	/// (Alternatively, you can use [`Mesh::compute_smooth_normals`] to mutate an existing mesh in-place)
	/// (Alternatively, you can use [`Mesh::compute_face_weighted_normals`] to mutate an existing mesh in-place)

	/// An example that would weigh each connected triangle's normal equally, thus skewing normals
	/// An example that would weight each connected triangle's normal equally, thus skewing normals

Uh oh!

Add angle-weighted smooth normals implementation (#18383) #18552

Add angle-weighted smooth normals implementation (#18383) #18552

Conversation

Waridley commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing & Showcase

Migration Guide

Uh oh!

alice-i-cecile commented Mar 25, 2025

Uh oh!

IceSentry left a comment

Choose a reason for hiding this comment

Uh oh!

Waridley commented Mar 26, 2025

Uh oh!

IceSentry commented Mar 26, 2025

Uh oh!

IceSentry commented Mar 31, 2025

Uh oh!

Waridley commented Mar 31, 2025

Uh oh!

IceSentry commented Mar 31, 2025

Uh oh!

Waridley commented Mar 31, 2025

Uh oh!

Waridley commented Mar 31, 2025

Uh oh!

greeble-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greeble-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Waridley commented Mar 25, 2025 •

edited

Loading