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.

Sign up for GitHub

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

Jump to bottom

Show visual-oriented 3D node gizmos only when selected #75303

Merged
merged 1 commit into from
Sep 15, 2023
Merged

Show visual-oriented 3D node gizmos only when selected #75303

merged 1 commit into from
Sep 15, 2023

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Mar 25, 2023 •
edited
Loading

Affected nodes:

  • DirectionalLight3D, OmniLight3D, SpotLight3D
  • ReflectionProbe
  • LightmapGI
  • VoxelGI
  • GPUParticles3D (but not collision/attractor nodes)
  • AudioStreamPlayer3D

This reduces visual clutter in the editor with 3D scenes.

Note that when selecting multiple nodes, none of them is considered as selected for gizmo drawing. This is not a new issue, but it'll become more noticeable with this PR.

Use https://github.com/godotengine/godot/pull/75303/files?w=1 to review changes (ignore whitespace changes).

Before

Before

After

After

@Calinou Calinou requested a review from a team as a code owner March 25, 2023 02:47
@Calinou Calinou added this to the 4.x milestone Mar 25, 2023
@Calinou Calinou force-pushed the 3d-gizmos-only-selected branch 2 times, most recently from cf29497 to d8fcdfe Compare March 25, 2023 02:59
@Calinou
Copy link
Member Author

Calinou commented Sep 14, 2023 •
edited
Loading

Rebased and tested again, it works as expected. AudioStreamPlayer3D and GPUParticles3D are now also affected by this change.

@WickedInsignia By the way, I wonder if this should apply to the Decal node gizmo as well. Being a purely visual element, it might get in the way if you have a lot of static decals placed in your scene. If we do that, there won't be any visual representation when not selected, but #81554 will address this (also for FogVolume, which also draws gizmo lines).

@Calinou Calinou changed the title Display Light3D/ReflectionProbe/VoxelGI/LightmapGI gizmos only while selected Show Light3D/ReflectionProbe/VoxelGI/LightmapGI/AudioStreamPlayer3D gizmos only when selected Sep 14, 2023
@Calinou Calinou force-pushed the 3d-gizmos-only-selected branch 2 times, most recently from 5ca778e to b186629 Compare September 14, 2023 23:59
@Calinou Calinou changed the title Show Light3D/ReflectionProbe/VoxelGI/LightmapGI/AudioStreamPlayer3D gizmos only when selected Show visual-oriented 3D node gizmos only when selected Sep 14, 2023
@Calinou
Show visual-oriented 3D node gizmos only when selected
1024ba0 
Affected nodes:

- DirectionalLight3D, OmniLight3D, SpotLight3D
- ReflectionProbe
- LightmapGI
- VoxelGI
- GPUParticles3D (but not collision/attractor nodes)
- AudioStreamPlayer3D

This reduces visual clutter in the editor with 3D scenes.
@WickedInsignia
Copy link

WickedInsignia commented Sep 15, 2023 •
edited
Loading

@WickedInsignia By the way, I wonder if this should apply to the Decal node gizmo as well.

Haven't used the Decal node myself before but just gave it a play-around, and that could be optimal. The gizmo doesn't seem to be selectable and you need to use the Scene Outliner to select decals anyway, so there's no real loss in function if that's hidden.
It seems that Unity keeps the bounding box for these visible at all times though.
It's not too big a deal that we keep these visible if there's an easily accessible "gizmo" toggle that turns all gizmos in the scene on/off. Maybe that functionality already exists but I haven't found it, and this would be extremely useful if placed in the 3D editor toolbar.
Not sure I can be the one to decide the best choice around the decal node considering how little I've used it though. I think it's easy enough to turn off via the existing show/hide in this case, and no functionality is lost if it's used that way (unlike turning off the gizmo to something like Lights).

@Calinou
Copy link
Member Author

Calinou commented Sep 15, 2023 •
edited
Loading

It's not too big a deal that we keep these visible if there's an easily accessible "gizmo" toggle that turns all gizmos in the scene on/off. Maybe that functionality already exists but I haven't found it, and this would be extremely useful if placed in the 3D editor toolbar.

This is already available with View Gizmos in the Perspective menu, but it should be exposed with a more visible button (and a shortcut) indeed. This is a per-viewport setting, which means it can be changed independently of other viewports when you have more than one viewport visible. I guess it'd need a button located on the left of the Perspective menu.

YuriSizov
YuriSizov approved these changes Sep 15, 2023
View reviewed changes
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I don't mind giving this a go, it looks reasonable.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Sep 15, 2023
AThousandShips
AThousandShips reviewed Sep 15, 2023
View reviewed changes
editor/plugins/gizmos/light_3d_gizmo_plugin.cpp
const Ref<Material> lines_billboard_material = get_material("lines_billboard", p_gizmo);
const Ref<Material> icon = get_material("light_omni_icon", p_gizmo);
if (p_gizmo->is_selected()) {
// Use both a billboard circle and 3 non-billboard circles for a better sphere-like representation
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
// Use both a billboard circle and 3 non-billboard circles for a better sphere-like representation
// Use both a billboard circle and 3 non-billboard circles for a better sphere-like representation.

editor/plugins/gizmos/light_3d_gizmo_plugin.cpp
Vector<Vector3> points_billboard;

for (int i = 0; i < 120; i++) {
// Create a circle
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
// Create a circle
// Create a circle.

editor/plugins/gizmos/light_3d_gizmo_plugin.cpp
const Point2 a = Vector2(Math::sin(ra), Math::cos(ra)) * r;
const Point2 b = Vector2(Math::sin(rb), Math::cos(rb)) * r;

// Draw axis-aligned circles
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
// Draw axis-aligned circles
// Draw axis-aligned circles.

editor/plugins/gizmos/light_3d_gizmo_plugin.cpp
points.push_back(Vector3(a.x, a.y, 0));
points.push_back(Vector3(b.x, b.y, 0));

// Draw a billboarded circle
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
// Draw a billboarded circle
// Draw a billboarded circle.

editor/plugins/gizmos/light_3d_gizmo_plugin.cpp
float d = r * Math::cos(Math::deg_to_rad(sl->get_param(Light3D::PARAM_SPOT_ANGLE)));

for (int i = 0; i < 120; i++) {
// Draw a circle
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
// Draw a circle
// Draw a circle.

editor/plugins/gizmos/light_3d_gizmo_plugin.cpp
points_primary.push_back(Vector3(b.x, b.y, -d));

if (i % 15 == 0) {
// Draw 8 lines from the cone origin to the sides of the circle
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
// Draw 8 lines from the cone origin to the sides of the circle
// Draw 8 lines from the cone origin to the sides of the circle.

@YuriSizov YuriSizov merged commit 508a758 into godotengine:master Sep 15, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks! I've decided to merge this despite the latest review comments because technically this PR didn't touch those lines, only changed the indentation. It would be good to have a consistency, but this is very minor.

@Calinou Calinou deleted the 3d-gizmos-only-selected branch September 15, 2023 18:29
@golddotasksquestions
Copy link

Is there any way to be able to see where the probes are placed now?

@Calinou
Copy link
Member Author

Calinou commented Sep 19, 2023

Is there any way to be able to see where the probes are placed now?

Yes, as wireframe outlines are visible when the node is selected (along with gizmo icons for LightmapProbe nodes).

@atirut-w
Copy link
Contributor

Any chance this will be cherry-picked for 4.1.x?

@akien-mga
Copy link
Member

akien-mga commented Oct 14, 2023 •
edited
Loading

4.2 will be released in less than a month, possibly before a potential 4.1.3. If you want usability enhancements such as this one, I suggest testing the betas.

@WickedInsignia
Copy link

@WickedInsignia By the way, I wonder if this should apply to the Decal node gizmo as well.

After playing around with the Decal node some more I believe it would be best if the bounds are hidden when the node is not selected. The bounds become a mess of wires fairly quickly and I can't identify much use in referencing the bounds when deselected. It appears Unity and Unreal hide them when deselected as well.

@Calinou Calinou mentioned this pull request May 19, 2024
Open
@Calinou Calinou mentioned this pull request Nov 6, 2024
Open
@Calinou Calinou mentioned this pull request Nov 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@YuriSizov YuriSizov YuriSizov approved these changes

Assignees
No one assigned
Labels
enhancement topic:editor topic:3d usability
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
7 participants
@Calinou @WickedInsignia @YuriSizov @golddotasksquestions @atirut-w @akien-mga @AThousandShips