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 helper for 3D gizmos and unify box #80278

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Aug 4, 2023

Follow-up to #71092
Closes godotengine/godot-proposals#5019

I added a new class similar to ViewPanner and modified every box gizmo I knew about.

godot.windows.editor.dev.x86_64_wug876f0K6.mp4

We could do the same refactor for other shapes too. The code duplication is huge.

Vector3 ray_dir = p_camera->project_ray_normal(p_point);

r_segment[0] = gi.xform(ray_from);
r_segment[1] = gi.xform(ray_from + ray_dir * 4096);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the value here. Some gizmos used 16k, but not all. Doesn't seem to affect editing, but maybe it's noticeable on very big scales, idk. It could be a parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Anything bad with defaulting to 16k?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea, hence I'm asking. I could change it to 16k.

@KoBeWi KoBeWi force-pushed the basically_ViewPanner_but_3D_and_without_panning branch 3 times, most recently from e679e2e to c75e589 Compare August 4, 2023 23:12
@fire
Copy link
Member

fire commented Aug 5, 2023

Wow, this is amazing. I'll check in later when the tests pass.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 5, 2023

Unfortunately I have no idea why the checks fail.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 6, 2023

Making the destructors of the classes using the helper seems to make the problem go away, I suspect it is due to compilation order, and that the first time the class destructor is referenced it is defined, causing problems if it is requested in a context where the include isn't present, the least invasive fix would be to add the constructors, haven't tested but compiling editor/plugins/gizmos before the editor/plugins might work as well

Edit: confirmed, this solves the compilation, will look into doing a PR to make this behaviour not context dependent across the engine

@KoBeWi KoBeWi force-pushed the basically_ViewPanner_but_3D_and_without_panning branch from c75e589 to 7fe526d Compare August 6, 2023 10:05
@AThousandShips
Copy link
Member

AThousandShips commented Aug 6, 2023

CollisionShape3DGizmoPlugin and CSGShape3DGizmoPlugin also need these

@KoBeWi KoBeWi force-pushed the basically_ViewPanner_but_3D_and_without_panning branch from 7fe526d to 0fd86f8 Compare August 6, 2023 10:31
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 6, 2023

It does not make any sense, but it seems to have helped.
Thanks for looking into it.

@AThousandShips
Copy link
Member

Looking into an alternative fix, general instead and unrelated to this PR, it's currently compiling, will see if it works

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 6, 2023

I think an alternative fix might be including gizmo helper in Node3D editor plugin.

compiling editor/plugins/gizmos before the editor/plugins might work as well

Order is enforced by formatting.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 6, 2023

I meant in the SCsub

My alternative fix unfortunately didn't work so will look at other cases to prevent the context dependent issue, ensuring that you don't run into a hard to diagnose error by having the result depend on which source is compiled first, so I'd say to keep the explicit destructors here

@KoBeWi KoBeWi force-pushed the basically_ViewPanner_but_3D_and_without_panning branch from 0fd86f8 to ae687da Compare August 17, 2023 14:42
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 good to me, though I didn't test. Might be worth rebasing.

@KoBeWi KoBeWi force-pushed the basically_ViewPanner_but_3D_and_without_panning branch from ae687da to 36681b6 Compare September 11, 2023 15:02
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 mostly works as expected. I noticed one issue related to this PR:

  • Decal does not use the new handler for resizing only one size, so it always behaves as if Alt as held down.

ReflectionProbe, VoxelGI and CSGBox3D all behave as expected.

Gizmo handle icons are also broken, but this also occurs in master:

image

@KoBeWi KoBeWi force-pushed the basically_ViewPanner_but_3D_and_without_panning branch from 36681b6 to 0c67124 Compare September 11, 2023 18:16
@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 11, 2023

Modified decal editor.

@akien-mga
Copy link
Member

In file included from ./core/io/file_access.h:36,
                 from ./core/io/config_file.h:34,
                 from ./editor/editor_plugin.h:34,
                 from editor/plugins/node_3d_editor_plugin.h:34,
                 from editor/plugins/node_3d_editor_plugin.cpp:31:
./core/object/ref_counted.h: In instantiation of 'void Ref<T>::unref() [with T = Gizmo3DHelper]':
./core/object/ref_counted.h:222:3:   required from 'Ref<T>::~Ref() [with T = Gizmo3DHelper]'
./editor/plugins/gizmos/decal_gizmo_plugin.h:38:7:   required from 'void memdelete(T*) [with T = DecalGizmoPlugin]'
./core/object/ref_counted.h:210:13:   required from 'void Ref<T>::unref() [with T = DecalGizmoPlugin]'
./core/object/ref_counted.h:222:3:   required from 'Ref<T>::~Ref() [with T = DecalGizmoPlugin]'
editor/plugins/node_3d_editor_plugin.cpp:7938:65:   required from here
Error: ./core/object/ref_counted.h:209:31: error: invalid use of incomplete type 'class Gizmo3DHelper'
  209 |   if (reference && reference->unreference()) {
      |                    ~~~~~~~~~~~^~~~~~~~~~~
In file included from editor/plugins/node_3d_editor_plugin.cpp:52:
./editor/plugins/gizmos/collision_shape_3d_gizmo_plugin.h:36:7: note: forward declaration of 'class Gizmo3DHelper'
   36 | class Gizmo3DHelper;
      |       ^~~~~~~~~~~~~

@KoBeWi KoBeWi force-pushed the basically_ViewPanner_but_3D_and_without_panning branch from 0c67124 to b61a4d1 Compare September 12, 2023 15:51
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.

Modified decal editor.

Works great now 🙂

@KoBeWi KoBeWi force-pushed the basically_ViewPanner_but_3D_and_without_panning branch from b61a4d1 to 015953a Compare September 12, 2023 16:08
@YuriSizov YuriSizov merged commit 7606221 into godotengine:master Sep 14, 2023
@YuriSizov
Copy link
Contributor

Thanks!

PS. Maybe for side handles we want a different icon? Like a similarly styled square instead of a circle?

@KoBeWi KoBeWi deleted the basically_ViewPanner_but_3D_and_without_panning branch September 14, 2023 13:34
@jcostello
Copy link
Contributor

@KoBeWi is it posible to make it work so the center of the node doesn't change when moving the gizmos? If so I can create a proposal. For things like Reflection Probes could be helpful so the offset and the center dont move when moving the gizmo

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 27, 2023

When you hold Alt while resizing the node stays in place.

@jcostello
Copy link
Contributor

@KoBeWi but both size resize evenly. Im talking about one side at the time

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 27, 2023

Eh I think I don't understand. You can open a proposal about it I guess and explain in details.
You can't move one side without moving the center (not sure about reflection probe, I think it had double gizmo).

@jcostello
Copy link
Contributor

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.

Add ability to move individual CSG faces as opposed to scaling from both sides. (and for CollisionShapes)
7 participants