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 Particle System emission shapes gizmo #86902

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paddy-exe
Copy link
Contributor

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

This Pull Request visualises the emission shapes from the GPU and CPU Particle Systems.

Closes godotengine/godot-proposals#7826

Showcase

image

Features

  • Handles for shapes manipulation
    • Sphere
    • Box
    • Ring
  • Shapes drawing
    • Box
    • Sphere
    • Ring
  • Emits a signal when either CPUParticles Node or the ParticleProcessMaterial resource of a GPUParticles Node properties are changed in any way so the changes can be synced and redrawn by the gizmo plugin

TODO

  • Creating a GPUParticles3D node in a blank scene results in the following error and no gizmo:
ERROR: Attempt to disconnect a nonexistent connection from '<ParticleProcessMaterial#-9223370086285254399>'. Signal: 'emission_shape_changed', callable: 'GPUParticles3D::update_gizmos'.
   at: _disconnect (./core/object/object.cpp:1492)
ERROR: Condition "material.is_null()" is true.
   at: _notification (./scene/3d/gpu_particles_3d.cpp:505)
  • Emission shape gizmos for unselected nodes should be hidden as to be less visually obstructive. (This is already done for the visibility AABB.)
  • Emission Shape Offset and Emission Shape Scale aren't taken into account by the handles
    • Sphere handling
    • Ring handling
    • Box handling
  • Fix Ring Axis is not properly being properly handled by the editor gizmo drawing when using non-uniform values

Massive thanks to @Arnklit for helping me with this Pull Request.

@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch 2 times, most recently from 94e1901 to 4363ca4 Compare January 6, 2024 22:48
@AThousandShips AThousandShips added this to the 4.x milestone Jan 6, 2024
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch 4 times, most recently from c9a3e02 to e659548 Compare January 9, 2024 20:01
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch from e659548 to cb39d7a Compare January 14, 2024 22:49
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch 2 times, most recently from 715a117 to 970796b Compare February 8, 2024 00:44
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch 3 times, most recently from 7cbb484 to abffa04 Compare April 16, 2024 14:04
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch from 5615a63 to b5d6302 Compare April 21, 2024 10:50
@paddy-exe paddy-exe marked this pull request as ready for review April 25, 2024 20:09
@paddy-exe paddy-exe requested review from a team as code owners April 25, 2024 20:09
@paddy-exe paddy-exe changed the title WIP: Add Particle System emission shapes gizmo Add Particle System emission shapes gizmo Apr 27, 2024
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch 3 times, most recently from cb575de to e3daec7 Compare April 29, 2024 12:13
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch from d2bc5e0 to 0afe343 Compare April 29, 2024 12:31
@paddy-exe
Copy link
Contributor Author

Thanks a lot for the style feedback @AThousandShips!

@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch from 0afe343 to 1afc432 Compare April 29, 2024 12:36
@akien-mga
Copy link
Member

@paddy-exe The 4.4 merge window is open, now would be a good time to finalize this PR.

@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch 3 times, most recently from c19cb27 to c71e2c4 Compare September 2, 2024 13:05
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch 2 times, most recently from c8ca5dc to 2f1391c Compare September 7, 2024 20:27
Co-authored-by: Kasper Frandsen <kasper.arnklit@gmail.com>
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch from 2f1391c to 46b896d Compare September 7, 2024 21:56
@paddy-exe
Copy link
Contributor Author

paddy-exe commented Sep 7, 2024

Alright, getting back into this PR again. Sorry for the waiting.
Here's an update:

  • Rebased on top of current master branch
  • figured out the error upon creating a new scene with GPUParticles3D Node and fixed it
image
  • Reworded the added documentation as requested
  • Emission Shape Offset and Emission Shape Scale aren't taken into account by the handles, even though the gizmo drawing is correct. This means the handles don't move to match the gizmo's actual position when these properties are set to non-default values. -> Still working on the handles as there are some discrepancies between the handles and the shape rendering for the ring shape
  • Emission shape gizmos for unselected nodes should be hidden as to be less visually obstructive. (This is already done for the visibility AABB.)

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

The description is very good, if there was any doubt.

@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch from 46b896d to 5749512 Compare September 9, 2024 13:47
@akien-mga akien-mga requested a review from QbieShay September 10, 2024 07:47
@QbieShay QbieShay self-assigned this Sep 10, 2024
@paddy-exe paddy-exe force-pushed the particle-emission-shapes branch from 5749512 to a6d6e33 Compare September 23, 2024 12:29
@akien-mga
Copy link
Member

You need to rebase the branch on latest master to fix the CI build, it's failing on some code not touched by this PR but which has been made invalid by #96532.


#include "particles_3d_emission_shape_gizmo_plugin.h"

#include "core/math/transform_3d.h"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#include "core/math/transform_3d.h"

Unnecessary

Vector3 s[2] = { gi.xform(ray_from), gi.xform(ray_from + ray_dir * 4096) };

Vector3 ra, rb;
float d;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
float d;
float d = 0.0f;

d = Math::snapped(d, Node3DEditor::get_singleton()->get_translate_snap());
}
d = MAX(d, 0.001);
// Times 2 because of using the half_height for drawing
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Times 2 because of using the half_height for drawing
// Times 2 because of using the half_height for drawing.

if (Object::cast_to<GPUParticles3D>(p_gizmo->get_node_3d())) {
GPUParticles3D *particles = Object::cast_to<GPUParticles3D>(p_gizmo->get_node_3d());

if (particles->get_process_material() != nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (particles->get_process_material() != nullptr) {
if (particles->get_process_material().is_valid()) {

for (int i = 0; i < 12; i++) {
Vector3 a, b;
box_aabb.get_edge(i, a, b);
// Multiplication by 2 due to the extents being only half of the box size
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Multiplication by 2 due to the extents being only half of the box size
// Multiplication by 2 due to the extents being only half of the box size.

// inner top ring cap
points.push_back(basis.xform(Vector3(inner_a.x, half_ring_height, inner_a.y)));
points.push_back(basis.xform(Vector3(inner_b.x, half_ring_height, inner_b.y)));
// inner bottom ring cap
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// inner bottom ring cap
// Inner bottom ring cap.

Point2 a = Vector2(Math::sin(ra), Math::cos(ra)) * ring_radius;
Point2 inner_a = Vector2(Math::sin(ra), Math::cos(ra)) * ring_inner_radius;

// outer 90 degrees vertical lines
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// outer 90 degrees vertical lines
// Outer 90 degrees vertical lines.

points.push_back(basis.xform(Vector3(a.x, half_ring_height, a.y)));
points.push_back(basis.xform(Vector3(a.x, -half_ring_height, a.y)));

// inner 90 degrees vertical lines
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// inner 90 degrees vertical lines
// Inner 90 degrees vertical lines.

Vector3 s_scale = mat->get_emission_shape_scale();

Vector3 ra, rb;
float d;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
float d;
float d = 0.0f;

d = Math::snapped(d, Node3DEditor::get_singleton()->get_translate_snap());
}
d = MAX(d, 0.001);
// Times 2 because of using the half_height for drawing
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Times 2 because of using the half_height for drawing
// Times 2 because of using the half_height for drawing.

@Repiteo Repiteo modified the milestones: 4.4, 4.x Jan 20, 2025
@KoBeWi
Copy link
Member

KoBeWi commented Feb 6, 2025

Looks like #102249 will supersede this?
The 3D part was already implemented in #100113

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Feb 6, 2025

Looks like #102249 will supersede this?

The 3D part was already implemented in #100113

No, the gizmo handles are still missing for 3d shapes. Will need to rebase this PR here but haven't gotten to it yet due to time constraints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Add particle system emission shape gizmos
10 participants