Disable normal raycaster for LOD generation by default #93727
Conversation
|
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 #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)
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). |
|
|
||
| int *ptrw = new_indices.ptrw(); | ||
| for (unsigned int j = 0; j < new_index_count; j++) { | ||
| vertex_corners[ptrw[j]].push_back(j); |
There was a problem hiding this comment.
note, ptrw[] here is already remapped by the shared loop above, so this is equivalent to old code that used vertex_corners[remapped] - no need to remap here again.
|
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
https://sketchfab.com/3d-models/chairmans-chair--20mb-02152d9f5083418399fbb5f64b129bf6
https://sketchfab.com/3d-models/half-life-alyx-skybox-building-3-2efb2bd5d137451486c9285bbf91228f
https://sketchfab.com/3d-models/mossberg-590-mariner-dc5b2d69587344229b60b1a7dbe07a81
https://sketchfab.com/3d-models/starship-troopers-arachnid-hopper-2429bfa8df1949c9af070ac445151a8f
|
|
Ok and final one... these take time to capture :D https://sketchfab.com/3d-models/tactical-axe-adc24921a6534ed8ba72dff3b8ec9fae
|
There was a problem hiding this comment.
@lyuma Can I have a second opinion on this?
I agree with the approach to this problem with the lod system.
Previously, I wasn't able to systematically make a justification for disabling the normal raycaster, now thanks to zeux we have some justification.
|
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: Before: Zeux changes: (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.) |
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 the long term plan would be to disable this feature entirely, the scripting API isn't changed, and it's just off-by-default there with no way to re-enable.
|
Updated the PR with a squashed commit (no code changes) |
There was a problem hiding this comment.
Tested locally (rebased on top of master f0d15bb), it works as expected.
Testing project: test_base_mesh.zip
Import speed seems to be ever so slightly faster. With this PR, it takes 21 seconds instead of 23 seconds to open the project with .godot/ removed, reimport all 3D scenes then exit the editor. (This is with the import speed-optimized editor settings.)
PC specifications
- CPU: Intel Core i9-13900K
- GPU: NVIDIA GeForce RTX 4090
- RAM: 64 GB (2×32 GB DDR5-5800 C30)
- SSD: Solidigm P44 Pro 2 TB
- OS: Linux (Fedora 39)
Strangely enough, primitive count when previewing the Camera3D in the scene hasn't changed at all with this PR:
| LOD disabled | Mesh LOD Threshold1 = 1.0 |
Mesh LOD Threshold = 1024.0 |
|---|---|---|
 |
 |
 |
Footnotes
-
Can be changed in the Project Settings. ↩
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.
|
There was a problem hiding this comment.
I think the PR looks great overall and I appreciate the careful testing.
I have raised this with the asset team on rocketchat and will continue the discussion there, but IMO we should just remove the raycaster code entirely. We added that code in the first place to mitigate what we saw as quality issues in vanilla MeshOptimizer. Now that MeshOptimizer is producing comparable quality normals, I would just remove it entirely
|
@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:
|
|
Thanks! |
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, in addition to increasing the vertex buffer size by ~15% due to vertex splitting. 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.