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

Explain parameter usage in GPUParticles3D and GPUParticles2D #85717

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

thmasn
Copy link
Contributor

@thmasn thmasn commented Dec 3, 2023

extended the description to explain how the passed parameters are used by the default ParticleProcessMaterial.
should set correct expectations for issues like 85358

@thmasn thmasn requested a review from a team as a code owner December 3, 2023 19:11
@AThousandShips AThousandShips added this to the 4.x milestone Dec 3, 2023
@thmasn thmasn changed the title Update GPUParticles3D.xml Explain parameter usage in GPUParticles3D.xml Dec 3, 2023
@thmasn
Copy link
Contributor Author

thmasn commented Dec 5, 2023

how can this simple text change fail a check?
the brackets are used in other descriptions too, there are no special characters.
i assume the failed check for linux w/ mono has an external reason?
this is my first commit here, so i am unsure how this works.

@akien-mga
Copy link
Member

akien-mga commented Dec 5, 2023

@thmasn See the review comment by @AThousandShips above. There are spaces at the end of the line that should be removed (and she suggested adding a word too, though that part is not what causes the CI error, the trailing spaces are).

Note: Please make modifications by amending the commit, and force pushing the changes, so that it stays as a single commit (see PR workflow). While doing this, you should also amend the commit message to be more explicit (e.g. like the PR title).

Alternatively, a maintainer can also update your PR branch directly to do those improvements.

@thmasn
Copy link
Contributor Author

thmasn commented Dec 5, 2023

i think i fixed it:

  • trailing spaces are removed
  • it is reduced to one commit with the correct title

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Dec 5, 2023
@akien-mga
Copy link
Member

There's a similar API in GPUParticles2D, so it sounds like the same changes should be made there?

Aside from my comment on the form, I can't really evaluate whether the actual content is accurate, would be good to get input from @QbieShay

As a side note, this custom parameter could now use Vector4 instead of Color. But that's outside the scope of this PR and probably not worth changing until Godot 5 to avoid unnecessarily breaking compatibility. But the use of Color as a container for 4 scalars predates the introduction of the Vector4 type.

@thmasn
Copy link
Contributor Author

thmasn commented Dec 5, 2023

i modified the text to also use the code-tag. i would favor the "use" word over "map", as the properties stay inside the vector4, and are maintained in this form.
and true, the way i understand it, GPUParticles2D uses the same ProcessMaterial, so the comment is valid for both.

i again amended the commit to match these changes.

@QbieShay
Copy link
Contributor

QbieShay commented Dec 5, 2023

It will overwrite both custom and color

@thmasn
Copy link
Contributor Author

thmasn commented Dec 5, 2023

It will overwrite both custom and color

yes, both are updated each frame in the processMaterial.

but, it seems to me like the intial parameters passed in custom still matter though: p.e. when you pass a lifetime of 0.0, or an age greater than the lifetime, the particle dies instantly.
on the other hand, the color that is beeing passed is instantly overriden, and therefore does not have any visible effect with the default processmaterial.

@QbieShay
Copy link
Contributor

QbieShay commented Dec 6, 2023

Oooh good catch!

@thmasn thmasn changed the title Explain parameter usage in GPUParticles3D.xml Explain parameter usage in GPUParticles3D.xml and GPUParticles2D.xml Dec 6, 2023
@YuriSizov YuriSizov changed the title Explain parameter usage in GPUParticles3D.xml and GPUParticles2D.xml Explain parameter usage in GPUParticles3D and GPUParticles2D Dec 8, 2023
@YuriSizov YuriSizov merged commit 7798ea7 into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks! And congrats on your first merged contribution to the engine!

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.

6 participants