Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit mesh complexity in LOD generation to prevent crashing #80467

Merged
merged 1 commit into from Sep 21, 2023

Conversation

aaronfranke
Copy link
Member

This PR fixes the crash in #80431. Instead of crashing, Godot will now print this message:

WARNING: Mesh LOD generation failed for mesh grass_Plane_009 surface 0, mesh is too complex. Some automatic LODs were not generated.
     at: generate_lods (scene/resources/importer_mesh.cpp:479)

I set the limit to 5000000 (5 million). I'm not certain what the limit should be, but a limit of 6 million crashes.

Note: I suspect that there is a bug in the LOD generator and that the limit should be higher than this. However, even if the limit should be higher, it's still good to have some kind of limit that prints a meaningful message instead of crashing. Also, this mesh lags Godot, so LODs are not the only problem with a mesh this big.

@Saul2022
Copy link

I guess for such kind of meshes the better could be mesh streaming as it designed for that.

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 10, 2023
@akien-mga akien-mga requested a review from a team August 10, 2023 06:50
@lyuma
Copy link
Contributor

lyuma commented Aug 10, 2023

I'm confused: The check is happening after the LOD was generated. If new_index_count > 5000000 then that means it was successful.

Shouldn't this be checking the index count before calling into SurfaceTool?

@clayjohn
Copy link
Member

I am also a little worried that this crash is VRAM amount dependent. The limit is likely different depending on your hardware

@Calinou
Copy link
Member

Calinou commented Aug 10, 2023

I am also a little worried that this crash is VRAM amount dependent. The limit is likely different depending on your hardware

The GPU used in #80431 has 8 GB of VRAM, so this will affect most users in this case. Even if you happen to have a GPU with more than 8 GB of VRAM, it's not guaranteed that everyone working on your project will also have a GPU with more than 8 GB of VRAM.

@fire
Copy link
Member

fire commented Sep 10, 2023

What feedback is required? Do we need to send 6 million triangle scenes and see what happens?

@aaronfranke
Copy link
Member Author

I don't know what people prefer as the optimal solution, but not crashing is better than crashing.

So if it's up to me, I would merge this to fix the immediate issue, and it can be improved later.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed during #animation / #asset-pipeline meeting. This is a workaround/hack to "fix" the issue until we actually find the issue. Only happens when you have a single mesh of ~200Mb.

@akien-mga akien-mga merged commit 184e603 into godotengine:master Sep 21, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the mesh-lod-limit branch September 21, 2023 16:10
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants