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

Avoid indexing instances without a base in scene cull phase #95503

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Aug 13, 2024

Fixes: #95466
Fixes: #92615
Fixes: #80749
Fixes: #69258
Fixes: #80504
Supersedes: #81069

At the core, this crash happens because an instance without a base is added to one of our BVHs and then we try to operate on that instance in the other BVH.

There are 2 main ways this crash occurs:

  1. The instance is updated while the base is still NONE. Then the base is set to a valid base type. And finally the base is set to an empty RID before the instance is updated. This can happen when freeing an instance the same frame it was created (Godot crashes during remove BVH leaf #69258) or by a Node that sets its base to RID() during update (CSG or Sprite3D both do this). This crash occurs
  2. The instance is updated while the base is still NONE, then the base is changed to a geometry type. This places the instance into the VOLUME BVH instead of the GEOMETRY BVH. And so the engine crashes when it tries to update the instance in the GEOMETRY BVH later.

In both cases, the problem comes from the fact that the instance is added to the VOLUME BVH. The logic is: if an object is a GEOMETRY type, it goes in the GEOMETRY BVH, otherwise it goes into the VOLUME BVH.

When we go to set the base of an instance, the logic is: if the previous base type is not NONE then remove the instance from the BVH and clear all data. If the base type is NONE we are assuming it was never added to a BVH. However, that assumption is false.

This PR fixes the bug by making that assumption true. It adds an early out to _update_instance to ensure that when the base type is NONE, we don't add the instance to a BVH. This change is low risk since the instance gets queued for update again once the base type is set. Without a base, the instance can't do anything. So its safe to skip the pairing/indexing step when the base has not been assigned.

The solution proposed by jsjtxietian in #81069 appears to work well for the Sprite3D case. It moves the instance from the VOLUME BVH to the GEOMETRY BVH if the base changes from NONE to GEOMETRY. However, it doesn't solve the other cases of the crash.

CC @jsjtxietian

@clayjohn clayjohn added bug topic:rendering crash cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Aug 13, 2024
@clayjohn clayjohn added this to the 4.4 milestone Aug 13, 2024
@clayjohn clayjohn requested a review from lawnjelly August 13, 2024 21:44
@clayjohn clayjohn requested a review from a team as a code owner August 13, 2024 21:44
Copy link
Contributor

@jsjtxietian jsjtxietian left a comment

Choose a reason for hiding this comment

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

LGTM. My pr is just a hack anyway.

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

LGTM, make perfect sense, there is no point adding this to either BVH as there is no data so I wonder why we added it originally.

@akien-mga akien-mga merged commit 032235b into godotengine:master Sep 3, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment