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

Sort single clicked UI element by z-index #96584

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

Sort single clicked UI element by z-index #96584

wants to merge 1 commit into from
+4 −0

Conversation

bikemurt
Copy link
Contributor

@bikemurt bikemurt commented Sep 5, 2024 •
edited
Loading

Edit: this is just a bandaid solution and the change (although helpful) needs to be weighed against the risk of putting things in the codebase that don't fix the full issue.

Fixes #96576
Small change, just need to sort the single-item selection possibilities by z-index before grabbing the 0th element.

@AThousandShips AThousandShips added bug topic:editor usability topic:2d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 5, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Sep 5, 2024
@kleonc
Copy link
Member

kleonc commented Sep 5, 2024

As mentioned in #96576 (comment), this won't make selecting work according to the rendering order in the general case. But as a band-aid fix for simple use case I guess it makes sense to select specifically according to the z-index.

While at this, note that after #68070 every CanvasItem has z-index, not only Node2Ds like before. So has_z can be removed here:

godot/editor/plugins/canvas_item_editor_plugin.h

Lines 266 to 273 in b6223c0

struct _SelectResult {
CanvasItem *item = nullptr;
real_t z_index = 0;
bool has_z = true;
_FORCE_INLINE_ bool operator<(const _SelectResult &p_rr) const {
return has_z && p_rr.has_z ? p_rr.z_index < z_index : p_rr.has_z;
}
};

Also here res.z_index = ci->get_effective_z_index() would make more sense:

godot/editor/plugins/canvas_item_editor_plugin.cpp

Lines 642 to 646 in b6223c0

_SelectResult res;
res.item = ci;
res.z_index = node ? node->get_z_index() : 0;
res.has_z = node;
r_items.push_back(res);

godot/scene/main/canvas_item.cpp

Lines 584 to 599 in b6223c0

int CanvasItem::get_z_index() const {
ERR_READ_THREAD_GUARD_V(0);
return z_index;
}
int CanvasItem::get_effective_z_index() const {
ERR_READ_THREAD_GUARD_V(0);
int effective_z_index = z_index;
if (is_z_relative()) {
CanvasItem *p = get_parent_item();
if (p) {
effective_z_index += p->get_effective_z_index();
}
}
return effective_z_index;
}

@bikemurt
Copy link
Contributor Author

bikemurt commented Sep 5, 2024

Yeah, I saw that regarding has_z as well.

I have not played around enough in the 2D part of the engine to know the other details. But from my understanding, the editor UI is simply clicking at a location, checking within a rect and grabbing all bottom-level nodes (non parents). Before we were just grabbing the 0th item off of that array - but it sounds like we need a general sorting algorithm that deals with z_index, show_behind_parent, etc. Presumably the renderer is doing that.

@bikemurt bikemurt mentioned this pull request Oct 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:editor topic:2d usability
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

2D selecting ignores z_index
3 participants
@bikemurt @kleonc @AThousandShips