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

Fix Vulkan crash with many Omni/SpotLights, Decals or ReflectionProbes #80845

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

bitsawer
Copy link
Member

When ClusterBuilderRD attempted to allocate memory for a buffer, it accidentally reserved memory for RenderElementData pointers instead of actual structures. This caused it to allocate much less memory than it thought, so when enough cluster elements were in a scene it overflowed the buffer. Easy to see with ASAN enabled in the issue MRP, if it doesn't crash after some camera movement resizing the window seems to trigger this bug immediately every time.

By default Max Clustered Elements in ProjectSettings is 512 items. This times 4 (clustered item types) is 2048 elements. sizeof(RenderElementData) is 80 bytes and pointer size is 8, so only 10% of needed memory is actually allocated. 10% of 2048 elements is roughly 204, and as Calinou and qarmin noted 103 (or 102) OmniLights and SpotLights (total of 206 elements) will start crashing as this is roughly where the buffer overflows the actual allocated size (204 elements) and causes an access violation or memory corruption.

@bitsawer bitsawer added this to the 4.2 milestone Aug 21, 2023
@bitsawer bitsawer requested a review from a team as a code owner August 21, 2023 10:01
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

It looks like all usages of render_elements assume that it is an array of elements, not an array of pointers to elements. So the initialization code is definitely wrong.

Great work tracking this issue down!

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 21, 2023
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (Linux + NVIDIA), it works as expected.

Creating 2,000 clustered elements in total with the default limit of 512:

Screenshot_20230821_163340

After increasing Max Clustered Elements to 8192 in the Project Settings:

Screenshot_20230821_163355

@akien-mga akien-mga merged commit b698631 into godotengine:master Aug 21, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

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