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

Add Heightfield mask to GPUParticlesCollisionHeightField3D #101947

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

Rudolph-B
Copy link
Contributor

Fixes #101750

As described in the issue, particles would collide with the icon of any node within the GPUParticlesCollisionHeightField3D AABB. Additionally, I noticed that an object's layer mask had no effect on whether it was considered by the height field.

This change addresses both issues.

@Rudolph-B Rudolph-B requested a review from a team as a code owner January 24, 2025 19:03
@Rudolph-B Rudolph-B force-pushed the Issue-101750 branch 2 times, most recently from d31771d to 0d00dc0 Compare January 24, 2025 19:12
@Rudolph-B
Copy link
Contributor Author

@clayjohn, I changed to comparing layer_mask against cull_musk . To accomplish this I needed to do a few extra things:

  • I needed to add functions to particle_storage to pull the cull_mask (I'm not really confident I followed standard here)
  • I needed to change the default value of the cull_mask to (1 << 20) - 1 (this excludes the gizmo and other similar layers)

@Rudolph-B Rudolph-B changed the title Fix GPUParticlesCollisionHeightField3D adding collisions excluded by its layer_mask Fix GPUParticlesCollisionHeightField3D adding collisions excluded by its cull_mask Jan 24, 2025
@Rudolph-B Rudolph-B requested a review from a team as a code owner January 24, 2025 19:48
@clayjohn
Copy link
Member

@clayjohn, I changed to comparing layer_mask against cull_musk . To accomplish this I needed to do a few extra things:

  • I needed to add functions to particle_storage to pull the cull_mask (I'm not really confident I followed standard here)
  • I needed to change the default value of the cull_mask to (1 << 20) - 1 (this excludes the gizmo and other similar layers)

You shouldn't change the default value to support this. You would be potentially breaking users' games in order to make something easier in the editor.

@Rudolph-B
Copy link
Contributor Author

I have removed the default value change.

I’m not entirely sure what happened. At some point during my testing, it was necessary to change the default value, but when I started testing again today, it was no longer required to fix the bug.

@Calinou
Copy link
Member

Calinou commented Jan 25, 2025

I can still reproduce the issue in the MRP with the latest revision of PR:

image

@Rudolph-B
Copy link
Contributor Author

I see, I just reloaded the stored cull_mask in the project instead of the default one.

@akien-mga akien-mga changed the title Fix GPUParticlesCollisionHeightField3D adding collisions excluded by its cull_mask Fix GPUParticlesCollisionHeightField3D adding collisions excluded by its cull_mask Jan 30, 2025
@Rudolph-B Rudolph-B requested a review from clayjohn February 1, 2025 18:35
@Rudolph-B Rudolph-B changed the title Fix GPUParticlesCollisionHeightField3D adding collisions excluded by its cull_mask Add Heightfield mask to GPUParticlesCollisionHeightField3D Feb 1, 2025
@Rudolph-B
Copy link
Contributor Author

As discussed in the rendering meeting I pivoted to adding a specific mask to GPUParticlesCollisionHeightField3D. This allows us not to break compatibility and to better match expected behavior.

I added @yahiaetman as a co-auther since I based this of their work in #93291

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.

Just some fixes for the documentation. The code looks great!

I wasn't able to reproduce the MRP, but I trust that you have tested carefully and can confirm that it is working properly

doc/classes/GPUParticlesCollisionHeightField3D.xml Outdated Show resolved Hide resolved
doc/classes/GPUParticlesCollisionHeightField3D.xml Outdated Show resolved Hide resolved
…its layer_mask

Co-authored-by: Yahia Zakaria <yahiazakaria13@gmail.com>
@Repiteo Repiteo merged commit ea2770e into godotengine:master Feb 4, 2025
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 4, 2025

Thanks!

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