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

Deprecate PrimitiveMesh.custom_aabb and ArrayMesh.custom_aabb #99345

Conversation

CrayolaEater
Copy link
Contributor

@CrayolaEater CrayolaEater commented Nov 17, 2024

There is a redundancy in the custom_aabb property shared by the PrimitiveMesh, ArrayMesh resources, and GeometryInstance3D.
I don't see the point of having 2 separate properties that do the same thing.
Also, GeometryInstance3D.custom_aabb can be visible because of this #89538.
image

Note: I also did a workaround for the PrimitiveMesh.custom_aabb and ArrayMesh.custom_aabb: when setting those properties now the GeometryInstance3D.custom_aabb will also update (making the custom_abb visible)

@CrayolaEater CrayolaEater requested review from a team as code owners November 17, 2024 03:51
@CrayolaEater CrayolaEater force-pushed the deprecated_mesh_resource_custom_aabb branch 2 times, most recently from 267c25f to d728715 Compare November 17, 2024 04:02
@Repiteo Repiteo added this to the 4.4 milestone Nov 17, 2024
@AThousandShips AThousandShips changed the title Deprecated PrimitiveMesh.custom_aabb and ArrayMesh.custom_aabb Deprecate PrimitiveMesh.custom_aabb and ArrayMesh.custom_aabb Nov 17, 2024
@CrayolaEater CrayolaEater force-pushed the deprecated_mesh_resource_custom_aabb branch from d728715 to a819d40 Compare November 17, 2024 17:35
@clayjohn clayjohn removed this from the 4.4 milestone Nov 17, 2024
@clayjohn
Copy link
Member

There is a very good reason to have custom_aabb on meshes. When a mesh uses a material that alters its size (for example by using the grow option in StandardMaterial3D or by using a ShaderMaterial), you need to use the custom_aabb to ensure the mesh AABB covers the entire mesh.

With this change, users would have to manually set a custom AABB on every single geometry instance that uses that mesh/shader. It is much better for users to set the custom_aabb once for the mesh and then have that automatically used by all GeometryInstances that use that mesh.

@CrayolaEater
Copy link
Contributor Author

CrayolaEater commented Nov 17, 2024

There is a very good reason to have custom_aabb on meshes. When a mesh uses a material that alters its size (for example by using the grow option in StandardMaterial3D or by using a ShaderMaterial), you need to use the custom_aabb to ensure the mesh AABB covers the entire mesh.

With this change, users would have to manually set a custom AABB on every single geometry instance that uses that mesh/shader. It is much better for users to set the custom_aabb once for the mesh and then have that automatically used by all GeometryInstances that use that mesh.

For the user to visualize the mesh it needs to be added to a mesh instance node anyway, so there will be a confusion to which custom_aabb to edit. There are also different scenarios which I'm not sure how the custom_aabb behaves (e.g. when you have a custom_aabb on the mesh resource and a different one set in GeometryInstance)

Right now I added a way for both properties to update when the resource custom_aabb changes, so should I remove the deprecation and only keep that?

I think the ideal way is to have the custom_aabb as a separate resource for each mesh to load separately. But thats way out of this PR scope.

@AThousandShips AThousandShips added this to the 4.x milestone Nov 17, 2024
@clayjohn
Copy link
Member

For the user to visualize the mesh it needs to be added to a mesh instance node anyway, so there will be a confusion to which custom_aabb to edit. There are also different scenarios which I'm not sure how the custom_aabb behaves (e.g. when you have a custom_aabb on the mesh resource and a different one set in GeometryInstance)

The AABB of the GeometryInstance is an AABB that surrounds all the AABBs of every surface. It is calculated by taking the min and max extents of all the surface AABBs.

This becomes really important for things like particles and multimesh, since the GeometryInstance AABB encapsulates all the AABBs of all the instances. Every time an instance moves you have to update the GeometryInstance AABB (which is every frame for particles). Using GeometryInstance.custom_aabb allows you to skip those calculations (which is the reason we added it).

Right now I added a way for both properties to update when the resource custom_aabb changes, so should I remove the deprecation and only keep that?

No, we need both properties.

@AThousandShips AThousandShips removed this from the 4.x milestone Nov 23, 2024
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.

4 participants