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 VoxelGI MultiMesh and CSG mesh baking #81616

Merged
merged 1 commit into from Oct 26, 2023

Conversation

bitsawer
Copy link
Member

Adds MultiMeshInstance3D::get_meshes() so that VoxelGI will collect the non-node geometry from MultiMeshInstance3D. That method is the semi-secret handshake that VoxelGI uses for this, currently GridMap, CSGShape3D and after this PR MultiMeshInstance3D support it. I'm not a big fan of that way of exposing this functionality, it's kind of limited, hard to extend and has to be made public only for this one usecase, but it's currently the supported way.

Also fixes a node GI mode check so that disabled, static and dynamic options work correctly. Bug #79987 was caused because this check was missing from one branch when collecting nodes during VoxelGI baking. This meant that CSGShape3D VoxelGI baking was wrong in following ways depending on its GI mode:

  • Disabled was actually Static
  • Static was Static (correct)
  • Dynamic was both Static AND Dynamic, which is not normally possible.

The last issue caused #79987 as the CSGShape3D was both baked as static while also considered dynamic, which caused some visual issues.

I also added the ERR_FAIL_COND_MSG() transform check to Voxelizer::plot_mesh() because of #75485. It's pretty easy to accidentally create NAN's and INF's when currently playing around with MultiMesh, this adds a simple validation check and skips the mesh with an error message if the transform is obviously wrong.

@fire
Copy link
Member

fire commented Sep 15, 2023

How would you recommend I test this?

@bitsawer
Copy link
Member Author

I think the MRP projects and their repro steps in the linked issues are a good start. You can also visualize the generated VoxelGI data in the 3D view by selecting Perspective -> Display Advanced -> VoxelGI Lightning after baking the VoxelGI.

Basically before this PR MultiMesh does not create any VoxelGI data when baked and CSG shape Global Illumination Mode is incorrect. After this PR the MultiMeshes should create VoxelGI representation and CSG shape mode should be correct. Also in both cases setting their node GI mode to disabled should also stop creating VoxelGI data for them and setting it to Static or Dynamic should enable it (after baking).

@bitsawer bitsawer requested a review from a team October 10, 2023 11:44
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 (rebased on top of master c5291a3), it works as expected.

image

out webp

@akien-mga akien-mga requested a review from a team October 10, 2023 20:27
scene/3d/voxel_gi.cpp Outdated Show resolved Hide resolved
@bitsawer
Copy link
Member Author

I kept the MultiMeshInstance3D::get_meshes() method and data format but removed the the scripting binding and instead call it directly as suggested. The voxel baker doesn't really have any mesh instancing support, we have to pass each mesh and transform separately and bake them individually anyway. However, this is pretty fast and only affects the baking step, this data doesn't exist during runtime.

I also noticed that GPUParticlesCollisionSDF3D uses similar logic and format too, so keeping the logic in one place in MultiMeshInstance3D is probably a good idea if we also want to make the particle collision generation support multimeshes in the future. However, that might need some planning to make sure people don't accidentally enable particle collisions with their million vertex grass multimesh or something like that.

Also rebased the PR and tweaked the is_node_voxel_bakeable() to return early in a few cases as suggested. I think current version is a pretty good, we can now also add/unify the particles multimesh support in another PR if needed without breaking any existing API's.

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.

Looks good to me

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 26, 2023
@akien-mga akien-mga merged commit 23b1f21 into godotengine:master Oct 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants