mesh: use faster acos() variant in normals calculation #114501

Aras Pranckevicius · 2023-11-05T12:54:21+01:00

2023-11-05 12:54:21 +01:00

acos() usage in all places related to normal calculations shows up in the profiler. Given that "angle between faces" is only additional heuristic weight in there (the effect of it at all is very subtle), approximate but faster version of acos() might be just fine. Especially since some other parts of Blender (e.g. mikktspace) use approximate acos in a conceptually the same part.

This PR:

Cleanup: removes saacos(), saasin(), sasqrt() since they were 100% identical to saacosf(), saasinf(), sasqrtf() since 2012.
Adds saacosf_approx() and saasinf_approx() to BLI_math_base.h. Implementation the same as already exists in Cycles; max error 0.00258 degrees.
Changes all normals related calculations to use the saacosf_approx() instead of saacosf().

Computing normals on a Stanford Lucy (14m verts) mesh:

Mac (arm64, M1 Max): 247ms -> 229ms
Win (x64, Ryzen 5950X): 276ms -> 250ms

All places that are about "normal calculation" were changed, including e.g. Corrective Smooth modifier. Applying that one to the same 14m vertices mesh, Mac M1 Max: 9.96s -> 9.76s

`acos()` usage in all places related to normal calculations shows up in the profiler. Given that "angle between faces" is only additional heuristic weight in there (the effect of it at all is very subtle), approximate but faster version of acos() might be just fine. Especially since some other parts of Blender (e.g. mikktspace) use approximate acos in a conceptually the same part. This PR: - Cleanup: removes `saacos()`, `saasin()`, `sasqrt()` since they were 100% identical to `saacosf()`, `saasinf()`, `sasqrtf()` since 2012. - Adds `saacosf_approx()` and `saasinf_approx()` to `BLI_math_base.h`. Implementation the same as already exists in Cycles; max error 0.00258 degrees. - Changes all normals related calculations to use the `saacosf_approx()` instead of `saacosf()`. Computing normals on a Stanford Lucy (14m verts) mesh: - Mac (arm64, M1 Max): 247ms -> 229ms - Win (x64, Ryzen 5950X): 276ms -> 250ms All places that are about "normal calculation" were changed, including e.g. Corrective Smooth modifier. Applying that one to the same 14m vertices mesh, Mac M1 Max: 9.96s -> 9.76s

❤️ 3 🚀 1

Aras Pranckevicius added 3 commits 2023-11-05 12:54:33 +01:00

acc40b6833 cleanup: saacos/saacosf, saasin/saasinf, sasqrt/sasqrtf have been identical for a decade

These all have had 100% identical implementations since 2012
(27e54f4d37), so remove one of them and switch everything to just
using the "f" suffixed ones.

ff086304ba math: add saacosf_approx / saasinf_approx

Implementation the same as fast_acosf / fast_asinf from Cycles. About
2x-4x faster, max error 0.00258 degrees.

buildbot/vexp-code-patch-coordinator Build done. Details

0896c8a0d2 mesh: use saacosf_approx in normal calculations

The "use angle between faces as extra weight" is only a heuristic to
improve certain cases of normals, and as such it probably does not
need extremely accurate "angle" calculation. So use approximate
but faster acos version. Especially since some parts of Blender
(e.g. mikktspace) already used exact same approximate acos.

Computing normals on a Stanford Lucy (14m verts) mesh:
- Mac (arm64, M1 Max): 247ms -> 229ms
- Win (x64, Ryzen 5950X): 276ms -> 250ms

All places that are about "normal calculation" were changed, including
e.g. Corrective Smooth modifier. Applying that one to the same 14m
vertices mesh, Mac M1 Max: 9.96s -> 9.76s

Hans Goudey reviewed 2023-11-05 22:12:49 +01:00

Hans Goudey left a comment

A few points from discussion so far, mostly as discussed in chat:

Using an approximation here seems reasonable (to me anyway!)
#113975 may be related, just something to keep in mind when figuring out the design here
#111718 impacts the performance characteristics of this area as well
Would be nice to switch to the C++ math API (BLI_math_base.hh instead of modifying the C API here
Because of that, seems clearer to me to handle the cleanup of saacos vs saacosf in a separate PR

A few points from discussion so far, mostly as discussed in chat: - Using an approximation here seems reasonable (to me anyway!) - #113975 may be related, just something to keep in mind when figuring out the design here - #111718 impacts the performance characteristics of this area as well - Would be nice to switch to the C++ math API (`BLI_math_base.hh` instead of modifying the C API here - Because of that, seems clearer to me to handle the cleanup of `saacos` vs `saacosf` in a separate PR

Brecht Van Lommel commented

2023-11-06 09:01:54 +01:00

@blender-bot build

Brecht Van Lommel approved these changes 2023-11-06 09:09:04 +01:00

Brecht Van Lommel left a comment

Looks fine to me assuming tests pass.

A further cleanup would be to rename sa -> safe_ and move them into BLI_math_base_safe.h, for consistency.

Looks fine to me assuming tests pass. A further cleanup would be to rename `sa` -> `safe_` and move them into `BLI_math_base_safe.h`, for consistency.

👍 1

Aras Pranckevicius commented

2023-11-06 09:44:30 +01:00

Would be nice to switch to the C++ math API

math_geom.c is still C tho :( Or should I move that to C++ before this, separately?

> Would be nice to switch to the C++ math API `math_geom.c` is still C tho :( Or should I move that to C++ before this, separately?

Hans Goudey commented

2023-11-06 11:10:34 +01:00

accumulate_vertex_normals_tri_v3, accumulate_vertex_normals_v3, and accumulate_vertex_normals_poly_v3 aren't used in any code that matters right now (only the second in metaball tessellation). So I'd just ignore that file for now.

`accumulate_vertex_normals_tri_v3`, `accumulate_vertex_normals_v3`, and `accumulate_vertex_normals_poly_v3` aren't used in any code that matters right now (only the second in metaball tessellation). So I'd just ignore that file for now.

Brecht Van Lommel commented

2023-11-06 11:48:45 +01:00

Why not update them anyway? If that code ever gets used or copied, we likely won't remember to apply this optimization.

Aras Pranckevicius commented

2023-11-06 12:06:58 +01:00

If there are no objections to moving math_geom.c to .cc then I can try doing just that. Does that need a PR/review, or "just do it"?

Hans Goudey commented

2023-11-07 09:36:01 +01:00

If there are no objections to moving math_geom.c to .cc then I can try doing just that. Does that need a PR/review, or "just do it"?

Usually a PR is helpful to make sure if compiles on all platforms. But it doesn't need review I don't think.

>If there are no objections to moving math_geom.c to .cc then I can try doing just that. Does that need a PR/review, or "just do it"? Usually a PR is helpful to make sure if compiles on all platforms. But it doesn't need review I don't think.

Aras Pranckevicius added 4 commits 2023-11-07 13:31:16 +01:00

7faaaa1c2a Revert saacos et al. cleanup (will do separately)

d16d140b26 Merge branch 'main' into acos_approx

a75c7b8638 math: add safe_acos_approx

Implementation the same as fast_acosf in Cycles. About 2x-4x faster,
max error 0.00258 degrees.

buildbot/vexp-code-patch-coordinator Build done. Details

9ce717b6f8 mesh: use safe_acos_approx in normal calculations

The "use angle between faces as extra weight" is only a heuristic to
improve certain cases of normals, and as such it probably does not
need extremely accurate "angle" calculation. So use approximate
but faster acos version. Especially since some parts of Blender
(e.g. mikktspace) already used exact same approximate acos.

Computing normals on a Stanford Lucy (14m verts) mesh:
- Mac (arm64, M1 Max): 247ms -> 229ms
- Win (x64, Ryzen 5950X): 276ms -> 250ms

All places that are about "normal calculation" were changed, including
e.g. Corrective Smooth modifier. Applying that one to the same 14m
vertices mesh, Mac M1 Max: 9.96s -> 9.76s

Aras Pranckevicius commented

2023-11-07 13:32:22 +01:00

@blender-bot build

Aras Pranckevicius commented

2023-11-07 14:32:47 +01:00

Looks fine to me assuming tests pass

@brecht so the tests do not pass, in exactly one place, which is Suzanne vertex normals. There's several vertices that result in a tiny bit normal difference (like 7th decimal after the dot). And in the OBJ exporter tests that emits only 4 normal digits, several vertices get that tiny difference. And in the modifiers test where Suzanne mesh gets a Displace modifier, some resulting vertices are different in position by up to 0.00002. Visually it looks the same and inspecting vertex normals in geometry nodes spreadsheet shows no difference (since it rounds before that).

I can update expectations of both tests, but the question is: is this tiny change in vertex normals acceptable or not?

> Looks fine to me assuming tests pass @brecht so the tests do not pass, in exactly one place, which is Suzanne vertex normals. There's several vertices that result in a tiny bit normal difference (like 7th decimal after the dot). And in the OBJ exporter tests that emits only 4 normal digits, several vertices get that tiny difference. And in the `modifiers` test where Suzanne mesh gets a Displace modifier, some resulting vertices are different in position by up to 0.00002. Visually it looks the same and inspecting vertex normals in geometry nodes spreadsheet shows no difference (since it rounds before that). I can update expectations of both tests, but the question is: is this tiny change in vertex normals acceptable or not?

Brecht Van Lommel commented

2023-11-07 15:59:58 +01:00

Yes, it's fine. You can just update the tests.

Aras Pranckevicius changed title from ~~WIP: mesh: use faster acos() variant in normals calculation~~ to mesh: use faster acos() variant in normals calculation

2023-11-07 18:18:12 +01:00

Aras Pranckevicius merged commit b1c7b573c0 into main

2023-11-07 18:22:25 +01:00

Aras Pranckevicius referenced this issue from a commit

2023-11-07 18:22:26 +01:00

mesh: use faster acos() variant in normals calculation

Aras Pranckevicius deleted branch acos_approx

2023-11-07 18:22:27 +01:00

Aras Pranckevicius referenced this pull request

2023-11-07 19:22:35 +01:00

cleanup: remove "sa" acos/asin/sqrt functions and use "safe_" variants #114593

Aras Pranckevicius referenced this issue from a commit