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 Bake Mask to GPUParticlesCollisionHeightField3D, Collision Mask to GPUParticles3D and Collision Layer to GPUParticlesCollision3D & GPUParticlesAttractor3D. #91858

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

yahiaetman
Copy link

@yahiaetman yahiaetman commented May 12, 2024

This pull request mainly adds a bake_mask field for GPUParticlesCollisionHeightField3D that functions similarly to the bake_mask in GPUParticlesCollisionSDF3D. It is used to filter which meshes are rendered in the height field texture.

However, my goal was to have separate collisions that reacts to different meshes and affects separate GPUParticles3D nodes. The problem was that the field cull_mask in GPUParticlesCollision3D did not work as noted in this issue #61014 (comment)

Looking at the code, the field cull_mask was not used anywhere, so I replaced it with collision_layer in both GPUParticlesCollision3D and GPUParticlesAttractor3D which uses a new set of layers under the name "layer_names/3d_particle_collision". For backward compatibility, the cull_mask property is kept as a deprecated alias to the collision_layer property. In the GPUParticles3D class, a corresponding field collision_mask was added. I argue that this would be easier for the users than using the 3D render layers for both the rendering culling masks and the 3D particle collision masks.

If any changes are needed or if you prefer that I split the GPUParticlesCollisionHeightField3D.bake_mask code to a separate pull request, please let me know.

And thank you for your amazing work on this wonderful game engine.

Change cull_mask in GPUParticleAttractor3D and GPUParticleCollision3D to collision layer.
Add new layer type for 3d particle collision.
Add collision_mask to GPUParticles3D.
@yahiaetman yahiaetman requested review from a team as code owners May 12, 2024 00:00
@yahiaetman yahiaetman changed the title Add Bake Mask to GPUParticlesCollisionHeightField3D, Collision Mask to GPUParticles3D and Collision Mask to GPUParticlesCollision3D & GPUParticlesAttractor3D. Add Bake Mask to GPUParticlesCollisionHeightField3D, Collision Mask to GPUParticles3D and Collision Layer to GPUParticlesCollision3D & GPUParticlesAttractor3D. May 12, 2024
@yahiaetman yahiaetman marked this pull request as draft May 12, 2024 10:51
@yahiaetman
Copy link
Author

I changed it temporarily to a draft till I get the checks to pass, then I will turn it to a pull request again as soon as possible.

scene/3d/gpu_particles_collision_3d.cpp Outdated Show resolved Hide resolved
scene/3d/gpu_particles_collision_3d.cpp Outdated Show resolved Hide resolved
scene/3d/gpu_particles_collision_3d.cpp Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

To handle the code style issues I'd suggest fixing it locally instead of relying on the CI, see here

@yahiaetman yahiaetman marked this pull request as ready for review May 12, 2024 15:14
@yahiaetman
Copy link
Author

I would like to open a discussion about which set of layers should be used for particle collisions. Currently, this pull request adds a new set of layers specifically for 3D particle collision, but I wonder if this is the best option. I think there are 3 options and these are the pros and cons of each in my opinion.

  1. 3D Render Layers: This was option used by the cull_mask property in both GPUParticlesCollision3D and GPUParticlesAttractor3D. It was supposed to be tested against the visual layer of the GPUParticles3D nodes.

    • Pros:
      • We don't need to add a new mask/layer property in GPUParticles3D.
    • Cons:
      • We need to access the visual layer from RenderingServer since it is not stored in the particle storage.
      • There are only 20 visual layers which are already used for other visual culling purposes, so it may become a limitation in large projects.
      • It is unintuitive to use layers that represent visual aspects of objects for physics related tasks.
  2. 3D Physics Layers: The GPUParticles3D class would have a collision_mask property that would be tested against a collision_layer property in both GPUParticlesCollision3D and GPUParticlesAttractor3D. We could also reverse them (put collision_layer in GPUParticles3D and the collision_mask in the collision and attractor nodes), but I think the first option is more intuitive.

    • Pros:
      • It is more intuitive to use physics layers for physics related tasks.
      • There are 32 available layers for physics, so it is less likely that it will be a limitation.
    • Cons:
      • We would be mixing between layers used for physics nodes and particle nodes.
  3. New 3D Particle Collision Layers: This is similar to option 2 but instead of using 3D Physics layers for the layer/mask properties, new layers are used. This is the option implemented in the pull request.

    • Pros:
      • It is more intuitive to use separate layers for particle collisions.
      • It would leave 32 available layers just for particle collisions and won't compete with visual or physics layers.
    • Cons:
      • It would add a new set of layers just for particle collisions which includes adding a new page in the project settings for these layers and more code to maintain down the line.

Personally, I am more inclined towards either option 2 or 3. What do you think?
Also would it be better to put the collision_mask in GPUParticles3D and the collision_layer in GPUParticlesCollision3D & GPUParticlesAttractor3D, or vice versa?

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.

Vulkan: GPUParticles3D attractor and collision Cull Mask doesn't have any effect
2 participants