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

Tree drag&drop line does not render correctly if an item has children #52053

Closed
Beider opened this issue Aug 24, 2021 · 5 comments · Fixed by #52096
Closed

Tree drag&drop line does not render correctly if an item has children #52053

Beider opened this issue Aug 24, 2021 · 5 comments · Fixed by #52096

Comments

@Beider
Copy link

Beider commented Aug 24, 2021

Godot version

3.3.2 stable mono win64

System information

Windows 10

Issue description

If you have a tree with items that have children and enable drag & drop with mode (DROP_MODE_INBETWEEN) then the drop hint line does not render correctly (see screenshot).

dragdroptree

As you can see the line renders correctly below the item that has no children (left side), however for the item with children it never renders below the item (right side).

That being said the position you get from "get_drop_section_at_position(position)" is correct and tells you that the item was dropped below the item even though the line is not rendered as such.

Steps to reproduce

Enable drag & drop in a tree that has items with children and try to drag something below an item with a child. I included a simple project that shows the issue.

Minimal reproduction project

Dragndroptree.zip

@kleonc
Copy link
Member

kleonc commented Aug 24, 2021

So I fixed the drawing (commit on my fork) but it doesn't solve the real issue about the current Tree's API:

version the look get_item_at_position(mouse_pos) get_drop_section_at_position(mouse_pos)
3.3.3.stable Godot_v3 3 3-stable_win64_2TdxmcUdNd TopLevel3 1 (Below)
3.x with fix uZGenkD4vy TopLevel3 1 (Below)

As you can see the problem which remains before/after the fix is that the user can't obtain the drop destination according to what's being drawn as a drop destination. So currently the user would need to duplicate the same logic in his drag-drop code to match what's being highlighted by the Tree.

A possible solution might be adding a method like get_drop_destination_for_position(position) which would return a Dictionary with item and section keys which would work like this:

version the look get_drop_destination_for_position(mouse_pos)
non-existent uZGenkD4vy {
"item": SeconLevel1,
"section": -1 (Above)
}

@Beider
Copy link
Author

Beider commented Aug 24, 2021

While this is very much an improvement, I agree with you that there are additional things to consider here.

So the question is where would the item end up when you drop over something with children? In my case I re-order items on the same level of the tree. As such perhaps the line when you drop on an item with children should actually be drawn below all the children and above the next item of the same level. That way if you want to drop it on that level of the tree it would indicate accurately where it would end up, however if you want to drop it as a child you would have to hoover the first child item.

So using your same drop location perhaps the line should be here instead,
dragdroptree2

With the solution provided above if we assume this is something like godot node tree, if the last item in the tree (item A) has children there is no way to indicate that you want to drop something on the same level as that item (item A) but after it. Since once you go past the last child (of item A) the line does not indicate that you are dropping on the parent level again.

I hope this made sense, I just think the only way to make sure any drop location works is to have it drop before/after the item on the same level of the tree (ignoring children).

@kleonc
Copy link
Member

kleonc commented Aug 24, 2021

Yes, what you're saying makes sense, I get your point.

However, just checked how Tree currently behaves in the Scene dock in the editor and... it behaves exactly according to what's being drawn with my fix. When dragging at "below" section of an item with children:
Godot_v3 3 3-stable_win64_xgYHve6lO2
although the line is being drawn above the hovered item, actual drop happens above the first child of the hovered item:
gmh2Q88vs3
In fact it's kinda reported in #46339, as clarified in this comment. So maybe I'll just make a PR with my fix just as a quick-fix for the current state of the editor's Scene dock. 🤔

This will of course not solve the issue I've mentioned in my previous comment: in drag-drop code user needs to perform the same logic which is applied when drawing to make things match. This needs to be somehow solved, I'd say this needs a separate proposal/discussion for finding a good solution.

About your point: when I was writing a fix linked above, I didn't even think about doing it like you're suggesting, the way it behaves currenly in the editor's Scene dock / the way it's drawn in my fix seemed obvious. So my doubt about your suggestion is: will it be intuitive? On the other hand if the indicators will be drawn correctly then the feedback for the user should make it obvious how things work. Not sure about it.
And currently in the editor's Scene dock to drop a Node at the end of some tree-level you can just drag Node at the parent of that level (hope it makes sense), for example:

dropping dropped
Godot_v3 3 3-stable_win64_16u21EHs1g GdWlRHXFrR
Cc6czQZZnj 2YCRF9PY3N

@Beider
Copy link
Author

Beider commented Aug 25, 2021

Thank you for the suggestion about the proposal, I filed one with my thoughts on how this should work. Hopefully this will at least start some sort of discussion for this so a solution can be found (even if it is not the one I suggested).

godotengine/godot-proposals#3201

@kleonc
Copy link
Member

kleonc commented Aug 28, 2021

Fixed by #52095/#52096.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants