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

Implement GPU Particle Collisions #42628

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

reduz
Copy link
Member

@reduz reduz commented Oct 8, 2020

-Sphere Attractor
-Box Attractor
-Vector Field
-Sphere Collider
-Box Collider
-Baked SDF Collider
-Heightmap Collider
sdfs


Vector<Vector3> points;

for (int i = 0; i <= 360; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Last line 360..361 overlaps with the first 0..1:

Suggested change
for (int i = 0; i <= 360; i++) {
for (int i = 0; i < 360; i++) {

Copy link
Member

Choose a reason for hiding this comment

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

Ping @reduz.

Copy link
Member

Choose a reason for hiding this comment

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

We have similar code with < and with <= in multiple places, sometimes even in the same file, if some cases do need this overlapping line, it probably should have comment why it's required:

for (int i = 0; i < 360; i++) {

for (int i = 0; i <= 360; i += skip) {

Copy link
Member

Choose a reason for hiding this comment

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

Could you open an issue about it? That's likely something for @JFonS to have a look at.

@Zireael07
Copy link
Contributor

As usual: Vulkan only or potentially backportable to GLES 3?

@Calinou
Copy link
Member

Calinou commented Oct 8, 2020

@Zireael07 This new functionality heavily relies on compute shaders. Most of this can't be done in GLES3.

@reduz
Copy link
Member Author

reduz commented Oct 8, 2020

@Zireael07 I think a good amount of the new particle work will probably work on GLES3, but will have to wait and see.

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 mostly good! Most of my comments are just about cleaning up code.

I noticed 3 issues while testing:

  1. If the GPUParticles3D is set to use local_coords then all collisions will be offset by the local transform of the GPUParticles3D.

  2. If the GPUParticlesCollisionHeightField is following the camera, it doesn't follow in the y direction. so if the HeightField is rotated it no longer follows the camera.

  3. when GPUParticlesCollisionHeightField is following the camera it is wasting a lot of resources by rendering the heightfield behind the camera, instead it should follow the position and rotation of the camera so that it is only covering a small area that is outside the camera's field of view.

Follow up tasks:

  • Marching cubes Gizmo to visualize SDF (@JFonS)

  • Debug mode for height_field_texture (Anyone)

  • Add NoiseTexture3D (Anyone)

-Sub Emitters
-Attractors
-Emitter positions deformable by bones
*Sub Emitters
Copy link
Member

Choose a reason for hiding this comment

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

Can delete these I guess now that they are done

@@ -172,6 +196,11 @@ bool emit_particle(mat4 p_xform, vec3 p_velocity, vec4 p_color, vec4 p_custom, u
return true;
}

float sd_round_box(vec3 p, vec3 b, float r) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks unused.


vec3 attractor_force = vec3(0.0);

if (PARTICLE.is_active) { //process collision
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this can be deleted, unless you want to do more with collision here?

depth = particle_size - s;
const float EPSILON = 0.001;
normal = mat3(FRAME.colliders[i].transform) * normalize(vec3(
texture(sampler3D(sdf_vec_textures[FRAME.colliders[i].texture_index], material_samplers[SAMPLER_LINEAR_CLAMP]), uvw_pos + vec3(EPSILON, 0.0, 0.0)).r - texture(sampler3D(sdf_vec_textures[FRAME.colliders[i].texture_index], material_samplers[SAMPLER_LINEAR_CLAMP]), uvw_pos - vec3(EPSILON, 0.0, 0.0)).r,
Copy link
Member

Choose a reason for hiding this comment

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

woooooah. Looks like we need to update our formatter tools

Copy link
Member

Choose a reason for hiding this comment

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

One needs to be a bit cleverer to avoid this. Should probably break manually before normalize.

col = true;
//normal = mix(normal,vec3(0,1,0),smoothstep(uvw_pos.y,local_y,y));
depth = dot(normal, pos1) - dot(normal, local_pos_bottom);
//depth = -depth;
Copy link
Member

Choose a reason for hiding this comment

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

Can clean up comments here

continue;
}

const float DELTA = 1.0 / 8192.0;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldnt this be 1.0/ size_of_height_field_texture?

Copy link
Member Author

Choose a reason for hiding this comment

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

problem is that the texture is not really po2 or square, maybe should just pass this a a vec2

@@ -6871,6 +7255,14 @@ bool RasterizerStorageRD::free(RID p_rid) {
_particles_free_data(particles);
particles->instance_dependency.instance_notify_deleted(p_rid);
particles_owner.free(p_rid);
} else if (particles_collision_owner.owns(p_rid)) {
ParticlesCollision *particles_collision = particles_collision_owner.getornull(p_rid);
//_particles_free_data(particles);
Copy link
Member

Choose a reason for hiding this comment

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

Can clean up

particles_collision->heightfield_fb = RD::get_singleton()->framebuffer_create(fb_tex);
particles_collision->heightfield_fb_size = size;

print_line("allooocc");
Copy link
Member

Choose a reason for hiding this comment

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

Can clean up


void GPUParticlesCollisionSDF::_find_closest_distance(const Vector3 &p_pos, const BVH *bvh, uint32_t p_bvh_cell, const Face3 *triangles, float thickness, float &closest_distance) {
if (p_bvh_cell & BVH::LEAF_BIT) {
//uint32_t triangle = p_bvh_cell & BVH::LEAF_MASK;
Copy link
Member

Choose a reason for hiding this comment

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

can clean up

@reduz
Copy link
Member Author

reduz commented Oct 9, 2020

@clayjohn some answers:

If the GPUParticles3D is set to use local_coords then all collisions will be offset by the local transform of the GPUParticles3D.

Seems like a bug, will give it a check.

If the GPUParticlesCollisionHeightField is following the camera, it doesn't follow in the y direction. so if the HeightField is rotated it no longer follows the camera.

This is intended, as the idea is that it works for open world, it only follows on XZ (local to the heighfield node). As this uses 32 bits floats for depth, you can specify a large vertical range anyway.

when GPUParticlesCollisionHeightField is following the camera it is wasting a lot of resources by rendering the heightfield behind the camera, instead it should follow the position and rotation of the camera so that it is only covering a small area that is outside the camera's field of view.

This is intended. The idea is that collision happens around the player and updates as you advance through the world.. If the lifetime of the particles is relatively long, you won t find collided particles when you turn around if this were the case.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Style nitpicks, but don't forget @bruvzg's comment which pinpoints a bug.

Comment on lines 70 to 72
}
Vector2 max(const Vector2 &p_vector2) const {
Copy link
Member

Choose a reason for hiding this comment

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

Add empty line between method declarations, even if they're related.

Comment on lines 103 to 111
bool is_working() const {
return current_work != nullptr;
}
uint32_t get_work_index() const {
return index;
}
void end_work() {
Copy link
Member

Choose a reason for hiding this comment

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

Add empty line between method declarations, even if they're related.

Comment on lines 2457 to 2459
//

////
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//
////
//
////
////////

or

Suggested change
//
////
//
////
//////

? ;)

We'll have to pick a style for this kind of separation comments eventually.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid using separation comments entirely, but that's just me. I feel like they don't help with readability significantly, while taking up vertical real estate.


Vector<Vector3> points;

for (int i = 0; i <= 360; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Ping @reduz.

Comment on lines 2715 to 2717
/////

////
Copy link
Member

Choose a reason for hiding this comment

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

🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

for <=360, i just copied from other gizmos, so ideally @JFonS should give these a check and see if they are correct or not and decide to change for a < in a separate PR

Comment on lines 564 to 569
}
Ref<Texture3D> GPUParticlesCollisionSDF::get_texture() const {
Copy link
Member

Choose a reason for hiding this comment

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

Add empty line between method declarations, even if they're related.

Comment on lines +786 to +791
}
GPUParticlesAttractor3D::~GPUParticlesAttractor3D() {
Copy link
Member

Choose a reason for hiding this comment

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

Add empty line between method declarations, even if they're related.

Comment on lines 812 to 818
}
GPUParticlesAttractorSphere::GPUParticlesAttractorSphere() :
Copy link
Member

Choose a reason for hiding this comment

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

Add empty line between method declarations, even if they're related.

virtual void particles_remove_collision(RID p_particles, RasterizerScene::InstanceBase *p_instance) = 0;

virtual void update_particles() = 0;
/* PARTICLES COLLISION */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* PARTICLES COLLISION */
/* PARTICLES COLLISION */

depth = particle_size - s;
const float EPSILON = 0.001;
normal = mat3(FRAME.colliders[i].transform) * normalize(vec3(
texture(sampler3D(sdf_vec_textures[FRAME.colliders[i].texture_index], material_samplers[SAMPLER_LINEAR_CLAMP]), uvw_pos + vec3(EPSILON, 0.0, 0.0)).r - texture(sampler3D(sdf_vec_textures[FRAME.colliders[i].texture_index], material_samplers[SAMPLER_LINEAR_CLAMP]), uvw_pos - vec3(EPSILON, 0.0, 0.0)).r,
Copy link
Member

Choose a reason for hiding this comment

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

One needs to be a bit cleverer to avoid this. Should probably break manually before normalize.

-Sphere Attractor
-Box Attractor
-Vector Field
-Sphere Collider
-Box Collider
-Baked SDF Collider
-Heightmap Collider
@akien-mga akien-mga merged commit b724d38 into godotengine:master Oct 9, 2020
@akien-mga
Copy link
Member

Thanks!

@aaronfranke
Copy link
Member

Where is the documentation for these features?

@reduz
Copy link
Member Author

reduz commented Oct 11, 2020

@aaronfranke The fact a feature is merged does not mean its final. Most of what I PRd will still have changes over the subsequent months. I always document the features I add sometime before the stable release. This is often the case with large features that are developed over time with partial commits.

@reduz
Copy link
Member Author

reduz commented Oct 11, 2020

@aaronfranke In any case, as I suggested you earlier, you need to be patient. The 4.0 branch is still under development and things are changing too much, don't expect the same rules that apply for the stable branches for the time being. The exception is only features that are clearly final, or features that will be cherry picked or backported, which is not the case of this one.

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.

7 participants