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

Move texture property back to the top level of the Particles inspector. #100227

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

clayjohn
Copy link
Member

We recently merged #95132 to re-order properties in the Particle2D inspectors for consistency with their 3D counterparts. In the midst of that, the texture property was also moved (despite not having an equivalent in 3D). Unfortunately, it was moved to be hidden under the new "Process Material" subsection even though it is unrelated.

This change adds confusion in two ways:

  1. In order to use the particle at all, it needs a texture, which means that users need to find the texture property in the last submenu which happens to have an unrelated name in order to use particles at all
  2. Its different from both GPUParticles3D (which has a mesh property in its own subsection) and CPUParticles2D (which has texture under the "drawing" section). So it moves us even further from being consistent despite that being the stated purpose of GPU/CPU particle parameter list consistency changes #95132

I suggest we bring the texture property back up to the top level and do the same with the CPUParticles2D to keep them consistent.

I suspect that having the process material in a submenu will annoy users as it adds a click for them just to set a property that we know in advance that they have to set. But I haven't seen any complaints, and it does match GPUParticles3D, so I left that alone.

This property needs to be set before the particles can be used. It should not be hidden away in an unrelated sub menu
@clayjohn clayjohn added this to the 4.4 milestone Dec 10, 2024
@clayjohn clayjohn requested a review from paddy-exe December 10, 2024 00:20
@clayjohn clayjohn requested a review from a team as a code owner December 10, 2024 00:20
@KoBeWi
Copy link
Member

KoBeWi commented Jan 1, 2025

This makes the process_material to be in 1-element group.

@clayjohn
Copy link
Member Author

clayjohn commented Jan 2, 2025

This makes the process_material to be in 1-element group.

@KoBeWi Yep! That is exactly how it is for GPUParticles3D too. Personally, I would have left the process_material out of a group. I noted my concern with it in the last paragraph of my original description:

I suspect that having the process material in a submenu will annoy users as it adds a click for them just to set a property that we know in advance that they have to set. But I haven't seen any complaints, and it does match GPUParticles3D, so I left that alone.

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 and makes sense👍🏻

@akien-mga akien-mga merged commit 763e8ce into godotengine:master Jan 13, 2025
20 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.

4 participants