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

Disallow selection of ownerless nodes #92188

Merged
merged 1 commit into from
May 28, 2024

Conversation

SaracenOne
Copy link
Member

Closes #92187

This changes the behaviour of node selection when selecting ownerless nodes in the 3D node editor. Now, instead of simply selecting the ownerless node, it will walk its parents and attempt to find the first node which is valid for selection.

@SaracenOne SaracenOne added this to the 4.3 milestone May 21, 2024
@SaracenOne SaracenOne requested a review from a team as a code owner May 21, 2024 05:01
@SaracenOne SaracenOne force-pushed the ownerless_node_select branch from 08d3d7c to 609df43 Compare May 21, 2024 05:18
@SaracenOne SaracenOne changed the title Disallows selection of ownerless nodes Disallow selection of ownerless nodes. May 21, 2024
@SaracenOne SaracenOne changed the title Disallow selection of ownerless nodes. Disallow selection of ownerless nodes May 21, 2024
@ryevdokimov
Copy link
Contributor

ryevdokimov commented May 21, 2024

I agree that having nothing shown as selected in the SceneTree is confusing, but maybe an alternative is to show some kind transient node selection that is denoted somehow (like yellow being nodes owned by another scene in the case of inherited scenes). My concern now is that you can't access and modify the ownerless nodes properties in the inspector, which might be desirable in some circumstance.

@SaracenOne
Copy link
Member Author

It's possible there is use for it, but looking everything which is generally considered standard operation in Godot, I really can't think of what that might be, and as pointed out in the issue, the current behaviour directly conflicts with a otherwise perfectly viable data-driven workflow. If the goal was to provide something similar with the behaviour of instanced scenes with the editable scene flag, I think at the very least there would be a requirement to flag an ownerless node as editable too before it can be selected.

@akien-mga akien-mga requested a review from KoBeWi May 22, 2024 06:38
@KoBeWi
Copy link
Member

KoBeWi commented May 22, 2024

This is consistent with 2D editor. The equivalent code is here:

if (p_node != scene && !p_node->get_owner()) {
return;
}
bool editable = p_node == scene || p_node->get_owner() == scene || p_node == scene->get_deepest_editable_node(p_node);

@caimantilla
Copy link

Aaah, I thought I was going crazy the other day! This happens with my spawner class, which instantiates the character scene to different positions for preview (orientation, entrance origin, destination, etc.)
Very keen on seeing this merged.

Attempts to select first node owned by the edited scene instead.
@SaracenOne SaracenOne force-pushed the ownerless_node_select branch from 609df43 to 596026a Compare May 28, 2024 10:13
@akien-mga akien-mga merged commit 25b17bd into godotengine:master May 28, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@mihe
Copy link
Contributor

mihe commented Jul 15, 2024

I stumbled upon a regression that seems to have been caused by this PR, where you're no longer able to use the list of selectable (overlapping) nodes feature in the 3D editor.

I put up a fix in #94387, in case anyone wants to take a look.

@mihe
Copy link
Contributor

mihe commented Jul 15, 2024

I'm also noticing that the code for this list selection feature now differs between 3D and 2D, where the 3D list selection now incorporates the logic from CanvasItemEditor::_find_canvas_items_in_rect that was mentioned above but the 2D list selection still just overwrites selection_results with the results from CanvasItemEditor::_get_canvas_items_at_pos, much like what the 3D editor did before.

Is that something that should be addressed?

EDIT: When taking a closer look at CanvasItemEditor::_get_canvas_items_at_pos, it seems to be doing some kind of filtering that's similar to what this PR introduces, so maybe it's fine?

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.

Node3D editor allows selection of ownerless nodes.
6 participants