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

GPU Particles (2D + 3D) visibility rect / gizmo optimization for reduced visual clutter #55052

Merged

Conversation

RPicster
Copy link
Contributor

@RPicster RPicster commented Nov 17, 2021

As I am dev with a very "visual and effects" heavy approach, I often use a lot of Particle effects.
This was first proposed here: godotengine/godot-proposals#3554

Problem in 2D

In the current version this often leads to very hard-to-understand scenes because of all the rendered visibility rects:
image
While this is an artificial example, my real-world examples are much, much worse.

Problem in 3D

While I did not do a full 3D project so far, I did one test scene in 4.0 and the gizmos of 3D Particle systems were a big annoyance to me. When I am building scenes I want a clear vision of how it will look in the game and big, colored gizmos overlaying the particle system I am working on in that moment are rather obstructive.

I found out that I can change the gizmo color in the editor settings, but it is not having any effect on the 'solid' color when selecting the gizmo.

My changes do two things

GPUParticles2D:

Only selected particles will show their visibility rect box, this reduces visual clutter on the stage.

bToQZtji4H.mp4

GPUParticles3D:

The 'solid' color takes the original gizmo color into consideration for the alpha.
Before a user could set the Particle3D gizmo color to transparent, but it would still always show the solid box upon selection because the alpha value was hardcoded to be set to 0.1.

With this change, the gizmo colors alpha from the editor setting will be taken into consideration and the alpha will be 0 when below a certain threshold.

SM52zZDS8Z.mp4

This is my first "real" PR and I'm a C++ newbie, so if there are things that could be improved, I'm happy to hear about it!

@GuyUnger
Copy link

i like this a lot, these boxes clutter up the scene without adding very valuable information when not working with them. hope it gets merged

@RPicster
Copy link
Contributor Author

Seems I forgot to include editor_node.h in the includes of the gpu_particles_2d header file...
Just pushed the fix to make the checks hopefully work.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 18, 2021

The include should be in either cpp or header, not in both. And cpp is preferred if possible, because includes in header cause unnecessary recompilation of files when any dependency is modified.

@RPicster
Copy link
Contributor Author

Ok, I notice I still have a lot to learn 🤣

The failed builds for the templates were caused by parts of the new additions not being wrapped in #ifdef TOOLS_ENABLED.
So parts were included when building the export templates.

I changed that and also tested building export templates using tools=no target=release which build without errors.

I also included the change requested by @KoBeWi

scene/2d/gpu_particles_2d.cpp Outdated Show resolved Hide resolved
@RPicster RPicster requested a review from a team as a code owner November 19, 2021 13:01
@RPicster
Copy link
Contributor Author

RPicster commented Nov 19, 2021

I started over with my implementation as I was pointed to the problem in regard to this decision: #29730

The visibility rect visibility is now controlled by the gpu_particle_editor_plugin and the dependecy to editor is removed from gpu_particle_2d

@RPicster RPicster requested review from Calinou and removed request for a team November 19, 2021 13:34
@RPicster
Copy link
Contributor Author

I unexposed the set_show_visibility_rect method from the scripting API, it wasn't neccessary and would create the need for documentation.

@RPicster RPicster requested a review from qarmin November 22, 2021 10:01
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks mostly good, see comments. The commits should also be squashed together, see PR workflow.

editor/plugins/gpu_particles_2d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/gpu_particles_2d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/gpu_particles_2d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/gpu_particles_2d_editor_plugin.cpp Outdated Show resolved Hide resolved
scene/2d/gpu_particles_2d.cpp Outdated Show resolved Hide resolved
scene/2d/gpu_particles_2d.h Show resolved Hide resolved
@RPicster RPicster force-pushed the master-particles-visibility-rect branch from b70394e to 4e948fb Compare November 25, 2021 19:39
@RPicster
Copy link
Contributor Author

Looks mostly good, see comments. The commits should also be squashed together, see PR workflow.

Alright! All commits are squashed and the review feedback was done!

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.

Looks ok, aside from 1 more nitpick.

@RPicster RPicster force-pushed the master-particles-visibility-rect branch from 4e948fb to bea8076 Compare November 25, 2021 21:47
@RPicster
Copy link
Contributor Author

Looks ok, aside from 1 more nitpick.

I'm thankful for all of your feedback! It's very new for me to write c++ code so I use every opportunity to learn!
I pushed the change :)

@RPicster RPicster force-pushed the master-particles-visibility-rect branch 2 times, most recently from b15b489 to 8b749c1 Compare November 28, 2021 14:13
… visual clutter in scenes with a lot of Particle nodes.
@RPicster RPicster force-pushed the master-particles-visibility-rect branch from 8b749c1 to 37cfa56 Compare November 28, 2021 18:43
@akien-mga akien-mga merged commit d2ac4bb into godotengine:master Nov 29, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@RPicster
Copy link
Contributor Author

Thanks! And congrats for your first merged Godot contribution 🎉

Thank you! Was a pleasure!

@RPicster RPicster deleted the master-particles-visibility-rect branch November 29, 2021 14:14
@RPicster
Copy link
Contributor Author

RPicster commented Dec 8, 2021

I was looking through my merged PRs for potential 3.x backports. I would love to have this in a future 3.x version - shall I create a backport? @akien-mga @KoBeWi

@akien-mga
Copy link
Member

Yeah that sounds good 👍

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.

7 participants