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

GPU/CPU particle parameter list consistency changes #95132

Merged

Conversation

LeonStansfield
Copy link
Contributor

@LeonStansfield LeonStansfield commented Aug 4, 2024

This draft PR introduces changes to the listing of parameters for the GPU and CPU particles 2D to be more consistent with their 3D counterparts as a minor quality of life improvement, as requested in #94687

The inconsistencies I noticed that have been addressed are:

On gpu_particles_2d:

  • interp_to_end are at different locations in the list between gpu_particles_3d and gpu_particles_2d
  • lifetime has exponential flag in its range slider on the gpu_particles_3d, which isn't the case in gpu_particles_2d.
  • preprocess has exponential flag in its range slider on the 3d version, which isn't the case in the 2d version
  • Draw order is set to index by default in gpu_particles_3d but lifetime in gpu_particles_2d

And on cpu_particles_2d:

  • lifetime has exponential flag in its range slider on the cpu_particles_3d, which isn't the case in cpu_particles_2d.

The changes should address these inconsistencies.

@LeonStansfield LeonStansfield marked this pull request as ready for review August 4, 2024 15:37
@LeonStansfield LeonStansfield requested a review from a team as a code owner August 4, 2024 15:37
@LeonStansfield
Copy link
Contributor Author

Hi everyone, is there any way to progress this PR getting reviewed and merged?

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.

Looks good to me overall.

My only concern is with moving process_material at the end of an already long list of settings. I guess that's how it is in 3D but I always found it's hard to find even though it's such an important (the most important probably) property for GPU particles.

CC @QbieShay @paddy-exe

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 26, 2024
@akien-mga
Copy link
Member

BTW while we wait for a second review, could you squash the commits? See PR workflow for instructions.

@QbieShay
Copy link
Contributor

I see in the original PR comment you list a change of default for draw order, but I cannot see it in the PR.

I don't think we should change it.

@AThousandShips
Copy link
Member

I see in the original PR comment you list a change of default for draw order, but I cannot see it in the PR.

Resolved above in a review comment

Copy link
Contributor

@paddy-exe paddy-exe left a comment

Choose a reason for hiding this comment

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

LGTM. There could be people complaining that the order will be confusing but I guess it makes sense to make them consistent.
Regarding loosing the overview: Wasn't there a proposal/discussion to have a default ParticleProcessMaterial when adding a GPU/CPU Particles Node to the SceneTree?

@akien-mga akien-mga merged commit 53f30bf into godotengine:master Oct 1, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

GPUParticles2D and GPUParticles3D have inconsistent property order in inspector
6 participants