Add angle-weighted smooth normals implementation (#18383) #18552
Conversation
alice-i-cecile
added
A-Rendering
There was a problem hiding this comment.
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.
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 |
|
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. |
Waridley
force-pushed
the
angle_weighted_normals
branch
2 times, most recently
from
aa4e9f5 to
6244acd
Compare
|
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 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. |
|
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. |
Waridley
force-pushed
the
angle_weighted_normals
branch
from
6244acd to
870c8e4
Compare
# 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
force-pushed
the
angle_weighted_normals
branch
from
870c8e4 to
5ed4b76
Compare
Waridley
force-pushed
the
angle_weighted_normals
branch
from
5ed4b76 to
470b5bd
Compare
|
Migration guide might be a bit wordy, feel free to give feedback. |
Waridley
force-pushed
the
angle_weighted_normals
branch
from
470b5bd to
79a6103
Compare
|
I really need to figure out why CI is behaving differently for me locally 😅 I'll have to fix the markdown errors later today. |
Waridley
force-pushed
the
angle_weighted_normals
branch
from
79a6103 to
a3af249
Compare
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_normalsmethod. I factored out the common code between both intoMesh::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_normalstests 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 forcompute_custom_smooth_normals.