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

[3.x] CPUParticles - fix non-interpolated NOTIFICATION_TRANSFORM #80827

Merged

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Aug 20, 2023

Updates global particle transforms immediately on receiving notification, to match legacy behaviour.

Fixes #80697

Notes

  • Godot 3.5 and earlier worked in this way. It isn't super efficient as it often transforms the particles twice on a frame, but it fixes the regression and allows making more efficient versions at leisure.
  • One alternative way to fix this is to disable GODOT_CPU_PARTICLES_2D_LEGACY_COMPATIBILITY, so global particles are specified in proper global space and there is no need to "re-jig" when the parent object changes.

Discussion

The whole system of using the inverse transform I have confirmed was a historical bodge to work with the existing VisualServer. So there is the opportunity to move the legacy particles to true global space as used by physics interpolated global particles.

On the downside, the bounds calculation is more complex when using true global space, because bounds are expected in local space (see fixes here in #80887 for physics interpolated particles) so I'm of the opinion it probably makes more sense to get this working and tested with physics interpolation before attempting the same with non-interpolated particles.

For this reason I'm moving this out of draft as it should be a good fixup in the meantime.

@oeleo1
Copy link

oeleo1 commented Aug 21, 2023

@lawnjelly Well, you made me compile Godot on Windows just to test this patch 👍 and indeed, I can confirm that it fixes the particle glitches seen in the 3.6 beta3 release and reported in #80697. Modulo the one idle frame when toggling Local Coords on/off but as you explained it is not critical, beyond being annoying.

I guess I am now set to test other patches...

@lawnjelly
Copy link
Member Author

lawnjelly commented Aug 21, 2023

I noticed another bug while working on the disabled GODOT_CPU_PARTICLES_2D_LEGACY_COMPATIBILITY version, which seems to be a transform problem in the fountain particles when physics interpolation is on with your MRP (the fountain moves twice as far as it should). I'll hopefully be tracking this down soon.

Be sure to try with physics interpolation on as well and report any strange behaviour. 👍

@lawnjelly
Copy link
Member Author

lawnjelly commented Aug 22, 2023

Have figured out the bug above with interpolated particles. I had previously been testing particle systems that were moved by their parent, and thus their local transform was 0.

When moving the particle system directly (in this case the fountain), the local transform was changing too, and I had been applying this in visual server (so the particles were moving twice as far as they should). As the particles are already in global space, the obvious solution is not to apply either parent or local transform.

It has occurred to me we should also pass on the regular final_transform to any children, just in case anyone added a child to a particle system (so to the child it would appear as though the transforms were propagated normally).

I'll do a PR for this bug. 👍

Updates global particle transforms immediately on receiving notification, to match legacy behaviour.
@akien-mga akien-mga merged commit 9eb4d5d into godotengine:3.x Sep 18, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the fix_cpu_particles_notif_transform branch September 18, 2023 15:08
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.

3 participants