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

Add visualization of 3D particle emission shapes #100113

Conversation

paddy-exe
Copy link
Contributor

@paddy-exe paddy-exe commented Dec 6, 2024

Adds support for visualization of particle emission shapes.

Separated part of #86902 due to discussion with @QbieShay on RocketChat

TODO

  • Implement support for ring_cone_angle (with support of @Arnklit)

Showcase

image

@paddy-exe paddy-exe added this to the 4.4 milestone Dec 6, 2024
@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from 7f3bcee to c9c468a Compare December 6, 2024 19:23
@QbieShay
Copy link
Contributor

QbieShay commented Dec 7, 2024

Should the signal emission be gated behind a tools build?

@paddy-exe
Copy link
Contributor Author

Should the signal emission be gated behind a tools build?

Hmm I guess we could put it behind a tools build flag 🤔 not sure if we have to though. Do you think we should?

@Calinou
Copy link
Member

Calinou commented Dec 8, 2024

Hmm I guess we could put it behind a tools build flag 🤔 not sure if we have to though. Do you think we should?

I'd say yes, if the signal has no purpose outside of the editor. If there's strong demand, the signal could be made available for use in projects later, but that would need a separate proposal to gauge interest first.

@QbieShay
Copy link
Contributor

QbieShay commented Dec 8, 2024

My rationale here is that I don't want this to cause performance issues in case people are animating the size (for whatever reason).

Alternatively instead of using a signal, it could be polled, but i don't know if gizmos support it.

@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from c9c468a to 6ed62f7 Compare December 10, 2024 14:04
@paddy-exe
Copy link
Contributor Author

@QbieShay I think I added this now

@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch 4 times, most recently from 7afaabb to b232c3a Compare December 12, 2024 17:08
@Arnklit
Copy link
Contributor

Arnklit commented Dec 13, 2024

The above suggestion should correct the ring gizmo. Needs to be added for CPU as well, but it should be identical.

godot.windows.editor.dev.x86_64_1KYwAabcbW.mp4

@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from 5d6653a to 7ab5283 Compare December 13, 2024 13:21
@paddy-exe
Copy link
Contributor Author

  • Added commit from Kasper
  • Added CPU Particle support for ring cone angle
image
  • Cleaned up the code comments

Ready for review now 👍🏻

@paddy-exe paddy-exe marked this pull request as ready for review December 13, 2024 13:24
@paddy-exe paddy-exe requested review from a team as code owners December 13, 2024 13:24
@paddy-exe paddy-exe requested a review from clayjohn December 13, 2024 13:28
@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch 2 times, most recently from a785ad6 to e9bf4f9 Compare December 14, 2024 03:45
@paddy-exe paddy-exe requested a review from a team as a code owner December 14, 2024 03:45
@paddy-exe paddy-exe requested a review from QbieShay December 14, 2024 13:44
@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from e9bf4f9 to 544ecef Compare December 16, 2024 02:25
@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from 544ecef to 3361073 Compare December 16, 2024 02:29
@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from 3361073 to 181a54a Compare December 16, 2024 12:12
@paddy-exe paddy-exe requested a review from Mickeon December 16, 2024 12:13
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Tested the PR locally, everything works except changing the emission shape offset/scale which emits the wrong signal (see comment below).

Small nitpick since this is undefined behavior; when setting the emission ring radius to 0 and the emission ring inner radius to something > 0, one of the rings disappears:

image

I mean we shouldn't allow this at all (add checks to the setters of ParticleProcessMaterial accordingly), but that's for another PR.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Looks great! Haven't tested but code looks good

@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from 181a54a to 90c544f Compare December 25, 2024 13:12
@paddy-exe paddy-exe requested a review from Geometror December 25, 2024 16:31
@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from 90c544f to a552883 Compare December 28, 2024 16:50
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Retested to ensure nothing broke. Great work!
Just a few missing consts left, then I think it's good to go.

Co-authored-by: Kasper Arnklit Frandsen <kasper.arnklit@gmail.com>
@paddy-exe paddy-exe force-pushed the particle-emission-shape-visual-separate-pr branch from a552883 to e689c12 Compare December 28, 2024 18:56
@paddy-exe
Copy link
Contributor Author

Done now and thanks for the detailed feedback 👍🏻

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected with both GPUParticles3D and CPUParticles3D.

Code and docs look good to me.

While testing, I found an unrelated bug: #100946

@akien-mga akien-mga merged commit d484e23 into godotengine:master Jan 3, 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.

10 participants