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 emission shape gizmos to Particles2D #102249

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

Arnklit
Copy link
Contributor

@Arnklit Arnklit commented Jan 31, 2025

This adds emission gizmos to CPU and GPU Particles2D. In combination with #100113 this would complete godotengine/godot-proposals#7826

godot.windows.editor.dev.x86_64_IFseWBzJ1M.mp4

I'd like some feedback or suggestion on what the best solution is for the gizmo color and look as it can look weird that there is another gizmo for the Visibility Rect on the GPU version.

Note: The Ring gizmo for GPUParticles will only display when the ring_axis is (1, 0, 0), (0, 1, 0) or (0, 0, 1) as it's not feasible to render gizmos for arbitrary axis in 2D.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 2, 2025

I think the gizmos should only draw when the node is selected, or at least there should be an option to hide them.

@Calinou
Copy link
Member

Calinou commented Feb 4, 2025

I think the gizmos should only draw when the node is selected, or at least there should be an option to hide them.

As I mentioned in #102198 (comment), the consensus nowadays is to show visual-oriented gizmos only when the node is selected, so I'd do that first.

If there's still a need to add a dedicated property to toggle gizmo visibility, we can do that later.

@Arnklit
Copy link
Contributor Author

Arnklit commented Feb 4, 2025

Thanks @KoBeWi for the review. Looking at the code I feel like it might make the most sense to use the show_visibility_rect bool for both the visibility rect and the emission gizmo, obviously it should then be renamed to show_gizmos instead or something along those lines. That bool gets set to true when selecting particle systems, so they would only show when selected and that would also remove them from runtime without having to add the is_editor_hint() guard

@Arnklit Arnklit force-pushed the particles2d-emission-shapes branch from 40031b1 to b329416 Compare February 6, 2025 17:02
@Arnklit
Copy link
Contributor Author

Arnklit commented Feb 6, 2025

@KoBeWi it should be much improved now if you have time to look it over again at some point :)

@Arnklit Arnklit force-pushed the particles2d-emission-shapes branch from b329416 to ab58e9c Compare February 6, 2025 17:19
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Functionality-wise looks fine.

@Arnklit Arnklit force-pushed the particles2d-emission-shapes branch from ab58e9c to 6feadd1 Compare February 7, 2025 16:13
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 (rebased on top of master), it mostly works as expected.

Testing project: test_particles2d_gizmos.zip

I noticed an issue with GPUParticles2D that has a box emitter. Its gizmo isn't at the correct position:

image

This doesn't occur with other emission shape types and occurs regardless of whether the scene root is transformed.

However, with CPUParticles2D, it occurs with all emission shape types when the scene root is transformed (like in the testing project):

image

image

Also, Ring emission shape in GPUParticles2D is only drawn if the axis is (0.0, 0.0, 1.0), but it should also draw when it's (0.0, 0.0, 0.0) with the same orientation:

image

There's a special behavior that makes the zero axis (which is not a valid unit vector) act like (0.0, 0.0, 1.0), so that it's easier to configure for 2D users.

@Arnklit Arnklit force-pushed the particles2d-emission-shapes branch from 6feadd1 to bbc549d Compare February 10, 2025 17:06
@Arnklit
Copy link
Contributor Author

Arnklit commented Feb 10, 2025

Rebased. And I think all the issues mentioned above are addressed now.

@Calinou thanks so much for the thorough testing, it made me realise there were a few more errors. I think they should all align now no matter how offset, rotated, scaled and skewed the particle nodes and their parents are :). Could you test again?

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.

Works great now 🙂

@Arnklit Arnklit force-pushed the particles2d-emission-shapes branch from bbc549d to 48c5100 Compare February 11, 2025 18:51
@KoBeWi KoBeWi modified the milestones: 4.x, 4.5 Feb 11, 2025
@Arnklit Arnklit force-pushed the particles2d-emission-shapes branch from 48c5100 to 3879703 Compare February 17, 2025 16:40
@Arnklit
Copy link
Contributor Author

Arnklit commented Feb 17, 2025

Realised I'd missed submitting one file for the typo correction. Should be all good now.

@Arnklit Arnklit force-pushed the particles2d-emission-shapes branch from 3879703 to 1322dd8 Compare February 18, 2025 11:27
@Arnklit Arnklit force-pushed the particles2d-emission-shapes branch from 1322dd8 to 03812fd Compare February 18, 2025 11:29
@Repiteo Repiteo merged commit e54a440 into godotengine:master Mar 11, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Mar 11, 2025

Thanks!

@Arnklit Arnklit deleted the particles2d-emission-shapes branch March 12, 2025 12:33
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