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

GPUParticles3D: Translate inactive particles to -INF #75162

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

ecmjohnson
Copy link
Contributor

Resolves #72650 by translating inactive particles to negative infinity. This solution was proposed by @Calinou in this comment. Note that this is an alternate to #75090 and only one is needed.

gpuparticles_billboarding_resolution2

@QbieShay
Copy link
Contributor

Thank you for the PR! Could you test with trails enabled?

@Calinou
Copy link
Member

Calinou commented Mar 21, 2023

Thank you for the PR! Could you test with trails enabled?

Trails will most likely exhibit the issue where they appear to go towards negative infinity (this already occurs with Hide on Contact collision). This needs a special case to avoid connecting the trail somehow, but I don't know how this could be done.

@ecmjohnson
Copy link
Contributor Author

I tried to setup a scene with trails enabled:

  1. create a new GPUParticles3D node in the scene
  2. under Trails check Enabled
  3. add a TubeTrailMesh (the caution symbol seemed to want this or RibbonTrailMesh) under Pass 1
  4. give it a new StandardMaterial3D
  5. under Billboard change Mode to Enabled
  6. under Transform check Use Particle Trails
  7. add a new ParticleProcessMaterial

Results before this change:

master_trails

Results after this change:

branch_trails

It doesn't appear that they are connecting to negative infinity or the origin, but I'm not totally sure what I'm seeing or if I've set them up properly. I'm not too familiar with the particles trails feature, or particles as a whole tbh. @QbieShay do you have a scene I can use for regression or point me to more information on GPUParticles3D trails?

@ecmjohnson
Copy link
Contributor Author

Is it expected that billboarding and trails features work together? They don't seem to behave as I would expect.

For an example trail particle system, with billboarding disabled:
trails
Becomes the following when billboard mode is set to Particle Billboard:
trails_billboard
It seems they aren't billboarding to face the camera:
trails_billboard_facing

@QbieShay
Copy link
Contributor

There's an extra trail property in the drawing tab of the particle system. Additionally trails need a skinned mesh. Setting them up is a bit complicated right now.

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! The same change is needed for the OpenGL renderer here:

mat4 txform = mat4(vec4(0.0), vec4(0.0), vec4(0.0), vec4(0.0)); // zero scale, becomes invisible.

I think once the fix is added to OpenGL we can go ahead and merge. I am not 100% sure this will fix the issue on all devices as -1.0 / 0.0 is not guaranteed to be inf it is simply undefined.

If we have issues with this fix we can switch to using #define FLT_MAX 3.402823466e+38 instead

@ecmjohnson
Copy link
Contributor Author

I actually did some research on the correctness of this method of generating an infinity before using it and it is correct for the GLSL #version 450 specified according to this answer; however, the change for GLES3 cannot use it reliably so for that I used the FLT_MAX approach instead. I verified that the GLES3 change resolved #72650 for the "Compatibility" renderer

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!

@akien-mga akien-mga merged commit e6e52f9 into godotengine:master Apr 13, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.3.

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.

GPUParticles3D flickers frozen particle at world origin (0, 0, 0) when shader material has y-billboard set
6 participants