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

Fix z-billboard + y to velocity transform alignment to correctly respect non-uniform scale #81315

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Fix z-billboard + y to velocity transform alignment to correctly respect non-uniform scale #81315

merged 1 commit into from
Sep 8, 2023

Conversation

conorlawton
Copy link
Contributor

Adds an alignment option for GPUParticles3D which does not alter the scale of the particle transform. This allows for manipulating the scale of the particle in the particle process material (e.g. being able to stretch it based on the particle's velocity).

@QbieShay
Copy link
Contributor

QbieShay commented Sep 5, 2023

Hey 👋 this should already be possible with what Godot provides. Can you supply an example project that uses it, highlighting which parts are dependant on this PR?

@conorlawton
Copy link
Contributor Author

Hi there, so the current z-billboard + y to velocity alignment (which should only control particle rotation) is averaging the scale of the particle. This causes some strange behaviour when trying to alter the particle scale from within the particle process shader.

With the change:
image

Without:
image

The custom part of the particle shader in the attached project.
image

Project:
Particles.zip

@QbieShay
Copy link
Contributor

QbieShay commented Sep 5, 2023

Ah. This looks like the flag is turning the non-uniform scale into uniform scale. I think this is a bug.

@conorlawton
Copy link
Contributor Author

I had considered removing the scaling from the existing alignment case but was afraid that it'd break existing effects for no real gain.

If you'd like, I could commit again to remove the case I added and remove the scaling on the existing case.

@QbieShay
Copy link
Contributor

QbieShay commented Sep 5, 2023

No, the existing one should keep scaling, but in fact it should keep scaling separately per-axis, while it seems that now it's using the same value for all axes.

@conorlawton
Copy link
Contributor Author

Sorry, by "remove the scaling" I meant removing the averaging and retain the scaling per axis.

@QbieShay
Copy link
Contributor

QbieShay commented Sep 5, 2023

Ah, then yes. I believe we should do that. I only have concerns regarding particle trails, because this option was meant initially for particle trails. It could break with nonuniform+trails, but we'll see. I pinged reduz on rocket chat about it.

But otherwise, then we agree. It should respect the nonuniform scale.

@QbieShay
Copy link
Contributor

QbieShay commented Sep 5, 2023

Could you test the interaction of nonuniform scale and particle trails?

@conorlawton
Copy link
Contributor Author

Particle trails scale as expected.
With non-uniform scaling:
image
Without (old):
image

Copy link
Contributor

@QbieShay QbieShay left a comment

Choose a reason for hiding this comment

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

Didn't test myself but the changes are good and we discussed both here and on RC.

@QbieShay QbieShay removed request for a team September 7, 2023 17:06
@akien-mga akien-mga changed the title Added no-scale z-billboard + y to velocity transform alignment Make scaling non-uniform for z-billboard + y to velocity transform alignment Sep 8, 2023
@akien-mga
Copy link
Member

I tried to rename the PR to be clearer about what it does, assuming I understood correctly. Could you also amend the commit similarly? It currently starts with "Removed additional case", which doesn't make much sense without context (I think it refers to previous work in this PR which has since been squashed?).

@QbieShay
Copy link
Contributor

QbieShay commented Sep 8, 2023

Yep you're correct. I'd say this is a fix actually. "Fixed z-billboard-y-velocity to correctly respect non-uniform scale instead of averaging scale"

@QbieShay QbieShay added the bug label Sep 8, 2023
@akien-mga akien-mga changed the title Make scaling non-uniform for z-billboard + y to velocity transform alignment Fix z-billboard + y to velocity transform alignment to correctly respect non-uniform scale Sep 8, 2023
@akien-mga akien-mga merged commit 3815b2f into godotengine:master Sep 8, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@conorlawton conorlawton deleted the z-billboard-y-velocity-no-scale branch April 19, 2024 14:38
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