godot
godot copied to clipboard
godotengine
Disable normal raycaster for LOD generation by default
Normal raycaster makes LOD generation process >2x slower and often generates normals that look significantly worse compared to what the simplifier comes up with by default. This was likely different before last meshoptimizer upgrade, as the attribute metric was not functioning properly, but now it looks like it's doing more harm than good.
This change makes it disabled by default but keeps an easy option to re-enable it per mesh using LOD parameters for now until we get more confidence and can remove the code outright.
Because it likely doesn't make sense to keep the raycaster forever, the scripting API isn't changed, and it's just off-by-default there with no way to re-enable.
Quality data, using the scene from #93587. In every screenshot the left hand side is the new behavior (raycast off), the right hand side is the current behavior (raycast on). The screenshots are generated by importing the scene again, and tweaking LOD bias for both meshes in concert until we see differences. Note, you will never see these up close, so I am sometimes intentionally picking very very very low quality LODs to show the delta.
Here raycast=off is much better:
Here they are about the same:
Here on the first pair (second-to-last LOD) raycast=off is much better; on the second pair (last LOD) raycast=off is better on the face but raycast=on handles the skirt a little better.
About the same here, maybe left is a little nicer?
Left (raycast=off) clearly better.
Left (raycast=off) a little better.
Left (raycast=off) is mostly better but the left side of the face is not ideal, that's probably a tie overall.
Both look good here.
Left (raycast=off) a little better on the ears in first pair; second pair (last LOD) left is better on the face.
Left (raycast=off) better.
Performance data, using a smaller variant of the scene in https://github.com/godotengine/godot/issues/93587 that has 100 meshes instead of 800 (trimmed duplicates).
Current import time: 37 seconds (according to --verbose); about 75% in generate_lods (according to perf) New import time: 22 seconds (according to --verbose); about 48% in generate_lods (according to perf)
37*0.75 = 27.75, 22*0.48 = 10.56, so ~2.6x faster LOD generation give or take overall. Most of the time in generate_lods is now spent inside meshopt_simplify, which can hopefully be improved in future PRs using a different simplification strategy to reduce repeated work.
Additionally, and maybe more importantly, the use of raycaster produces many new normal values that result in vertex splitting (as in, existing vertices get duplicated). The extra vertices will be part of the vertex arrays and will consume memory, increasing the amount of vertex data - on the scene from the referenced issue, the aggregate number of vertices with raycast=off (with this change) is 2.8M, but with raycast=on it's 3.3M (so 17% more GPU memory spent on vertex data).
As far as I can tell one of historical uses of this was to squeeze out more uniformly distributed lod levels all the way to destruction of the mesh.
We can revisit this!
Can we pick some meshes that are more human created these samples seem to have a wobbliness associated with video to mesh ai generation algorithms. I expect better results on artistic meshes! but for completeness I think we need to check.
Yeah that's fair. I grabbed a couple models from Sketchfab. Left = raycast off, right = raycast on.
https://sketchfab.com/3d-models/3dmodel-based-on-shinypants-by-alan-blackwell-29e4f890f9e140618142f774c11ef233 Here both models are decent but the raycast=on model has a couple cases where the normal is very incorrect, here on the visible leg (and similar artifact on the other side of the model).
https://sketchfab.com/3d-models/chairmans-chair--20mb-02152d9f5083418399fbb5f64b129bf6 Here I don't love either result :) but I guess raycast=off shading is smoother which may be less visually distracting in the distance. But neither is obviously better.
https://sketchfab.com/3d-models/half-life-alyx-skybox-building-3-2efb2bd5d137451486c9285bbf91228f Here I had to turn the house ~90 degrees (the front side is perfect on both), the left is still raycast=off - I think in raycast version the rays here hit the building at an odd angle or something and the normals get picked incorrectly.
https://sketchfab.com/3d-models/mossberg-590-mariner-dc5b2d69587344229b60b1a7dbe07a81 These are the same basically, I wasn't able to find any angle at which there's a meaningful difference.
https://sketchfab.com/3d-models/starship-troopers-arachnid-hopper-2429bfa8df1949c9af070ac445151a8f These are very close. It's hard to tell, I think raycast=on has harsher lighting as the normals have more creases but overall in LOD scenarios both are fine.
Ok and final one... these take time to capture :D
https://sketchfab.com/3d-models/tactical-axe-adc24921a6534ed8ba72dff3b8ec9fae This one is interesting because the handle is better processed by the raycast=on version, but the actual axe surface is better preserved by the raycast=off version, and it's larger so more visually apparent perhaps. But both are probably fine in the distance.
I do think this is good for 4.4, we're trying to slowdown merges for 4.3 to manage the release.
In our https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html workflow we tend to bias towards one commit unless rare circumstances. Can you squash?
Edited: I'm running the pr through my tests cases.
Unsure if this is helpful but my local release build tests: These tests were completed using Reimport for each file. As expected, the Hydralisk_obj file was unaffected. It is just one ginormous obj. Still awesome as a change just for perf, let alone visual fidelity.
Before: Godot Engine v4.3.beta.custom_build.811ce36c6 (2024-06-28 13:55:14 UTC) Vulkan 1.3.277 - Forward+ - Using Device #0: AMD - AMD Radeon RX 5700 XT EditorFileSystem: "res://chess_set.glb" import took 18322 ms. EditorFileSystem: "res://Hydralisk_obj.obj" import took 42330 ms. EditorFileSystem: "res://scene.glb" import took 440525 ms.
Zeux changes: Godot Engine v4.3.beta.custom_build.81bf8de4c (2024-06-29 02:02:55 UTC) EditorFileSystem: "res://chess_set.glb" import took 10499 ms. EditorFileSystem: "res://Hydralisk_obj.obj" import took 42996 ms. EditorFileSystem: "res://scene.glb" import took 234556 ms.
(For anyone curious about Hydralisk specifically, it appears to be an insanely detailed Hydralisk from Starcraft, made in Z-Brush. It's a 350MB mesh.)
primitive count when previewing the Camera3D in the scene hasn't changed at all with this PR
Yeah that's expected - the extra normal splits increase the vertex count (which the widget doesn't show and it's more of a factor wrt total mesh memory consumption), the primitive count should be consistent between the setting being off/on.
edit this is shown in the profiler under "Monitors -> Video -> Buffer Mem", and it's 64.39 MB after this change and 67.53 MB before this change, so ~5% savings overall here.
Also double checked that test file and the import performance seems reasonably in-line; this one has a very different performance profile, as it's using a lot of generally smaller .obj files instead of a big .gltf file; after the change, LOD generation only accounts for ~10% of the time, so if out of 21 seconds it's 2 seconds give or take, then before the change it would have been about twice that, for 2 extra seconds of import time.
@clayjohn My expectation was basically to do that as a second step in uhh 4.5 or something - my understanding is that neither this PR nor a future removal of the setting is technically a breaking change, other than it affecting a re-import of a given asset (in hopefully positive ways!), so this provides a smoother transition which gives some opportunity to discover cases where raycasted normals were better. I'm fine with either, the raycast code doesn't interfere with the rest of the flow thankfully, so it's not too difficult to keep working, but future changes to the simplifier may be harder to test because there's both modes to consider.
We discussed this a bit on the chat and I have changed my position somewhat.
I think ideally we would not have an import setting for this and I suspect that no one will notice the change anyway. However, I do think that zeux' suggestion to provide the setting for compatibility reasons makes sense.
Therefore, I suggest the following:
- Let's merge this PR as-is early in the 4.4 release cycle
- If users report bugs, we can advise them to enable the raycast option
- If a lot of users run into issues, we can make switch to enabling it by default before 4.4 releases
- If we don't get any bug reports, then we can just remove the setting altogether before 4.4 releases
