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 option to toggle always showing collision shapes #99569

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yahkr
Copy link
Contributor

@yahkr yahkr commented Nov 23, 2024

closes godotengine/godot-proposals#11220

Currently even a simple scene can be visually distracting with a few collision shapes:

This PR adds a setting to only show collision shapes when the node is selected vs all the time.

This is with no object selected

With this PR:

@yahkr yahkr requested review from a team as code owners November 23, 2024 02:02
@yahkr yahkr force-pushed the hide_colliders branch 2 times, most recently from 9aa24b5 to 51b7957 Compare November 23, 2024 02:18
@Chaosus Chaosus added this to the 4.4 milestone Nov 23, 2024
@AThousandShips AThousandShips modified the milestones: 4.4, 4.x Nov 23, 2024
@Calinou
Copy link
Member

Calinou commented Nov 23, 2024

I like the idea, but I'd say this would be better suited to the View menu at the top of the 3D editor viewport instead. This way, it'll persist on a per-scene basis and behave more like the other gizmo visibility options which are present in that menu.

@yahkr
Copy link
Contributor Author

yahkr commented Nov 23, 2024

I like the idea, but I'd say this would be better suited to the View menu at the top of the 3D editor viewport instead. This way, it'll persist on a per-scene basis and behave more like the other gizmo visibility options which are present in that menu.

Personally I know I will probably never have this option enabled. So having to set it per scene would be tedious IMO. Especially when importing many meshes and viewing their scenes. But if that's the recommended way forward I can make that change as I'd rather have the option than none at all.

@yahkr
Copy link
Contributor Author

yahkr commented Nov 23, 2024

@Calinou

So after exploring some I'm running into issues of how to connect that view menu setting to the gizmo logic. without access to the scene's state I can't look up that property.
image

One solution I have found is adding a fourth option to the gizmo visibility selector:
image

and then replace this check with the following:

ref->set_hidden(current_state == HIDDEN);

ref->set_hidden(current_state == HIDDEN || (!ref->is_selected() && current_state == SELECTED));

Thoughts on this vs the current PR in the Editor Settings/Editors/3d Gizmos/Gizmo Settings?

@yahkr
Copy link
Contributor Author

yahkr commented Dec 2, 2024

Sorry was away on holiday, hopefully this is more clear. The issue I was running into regarding putting it in the view menu, is how to access that property. Since it is stored within the editstate file of the scene itself.

I can't find a clean way of accessing Node3DEditorViewport:: view_menu->get_popup()->is_item_checked(...) from CollisionShape3DGizmoPlugin::redraw

as far as I can see the only route of communication between those settings is via

Ref<EditorNode3DGizmo> EditorNode3DGizmoPlugin::get_gizmo(Node3D *p_spatial) {

which is where the visibility selection is checked.

Solution 1:

Current PR, adding a new setting within editors/3d_gizmos/gizmo_settings/always_show_collision_shapes
IMO the least bloat and my preferred solution.

Solution 2:

adding a fourth option to the gizmo visiblity for only visible while selected (applies to all gizmos):
This would require some modification to change VISIBLE = ALWAYS VISIBLE, and setting defaults for gizmos to whatever they are now. ex: Light3DGizmo is selected only, Collision is always.

public:
static const int VISIBLE = 0;
static const int HIDDEN = 1;
static const int ON_TOP = 2;

Solution 3:

Exposing something akin to EDITOR_GET for the scenes edit state. or pulling the state with something like this:

	Node3DEditorViewport *vp = Node3DEditor::get_singleton()->get_last_used_viewport();
	Dictionary d = vp->get_state();
	if (d.has("always_show_collision_shapes")) {
		show_collision_shapes_always = d["always_show_collision_shapes"];
	}

but this feels rather messy

@KoBeWi
Copy link
Member

KoBeWi commented Dec 26, 2024

I think the option name is a bit unclear. "Always show", if you disable it, the shapes are going to show sometimes. When exactly?
Reversing it into show_collision_shapes_only_when_selected, while more verbose, is also more informative.

@yahkr
Copy link
Contributor Author

yahkr commented Dec 29, 2024

show_collision_shapes_only_when_selected

Totally agree with this change. Rebased to efa1443 & made this change :)

@yahkr yahkr force-pushed the hide_colliders branch 3 times, most recently from 581e1fb to 239b7c9 Compare December 29, 2024 19:22
@KoBeWi
Copy link
Member

KoBeWi commented Jan 21, 2025

The shapes are still visible without the gizmo:
image
Though it's probably expected?
Also you can't select them without a gizmo, but it's probably not a problem.

@Calinou
Copy link
Member

Calinou commented Jan 21, 2025

Though it's probably expected?

This is likely something that was forgotten during rebase: #90644 and #101810

@yahkr
Copy link
Contributor Author

yahkr commented Jan 22, 2025

Though it's probably expected?

This is likely something that was forgotten during rebase: #90644 and #101810

Thanks! Fixed & pushed

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 works as expected.

collision_shapes_selected_only.mp4

@KoBeWi KoBeWi modified the milestones: 4.x, 4.5 Jan 30, 2025
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 an editor setting to only show 3D collision shape gizmos for selected nodes
5 participants