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

Drag and Drop helper "reorder drop target" api #2823

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

Conversation

rokups
Copy link
Contributor

@rokups rokups commented Oct 2, 2019

This PR implements a convenience API for reordering of items we discussed in #143. In order to achieve good visuals i added ImGuiDragDropFlags_AcceptNoBorderPadding flag which disables expansion of rectangle marking drop target. I took a liberty to add a style variable as well. In my own project i simply commended out hardcoded padding as overall it looked better. I used ItemSpacing for constructing rectangle for reorders, however at times it can be quite hard to place items on. Not sure what to do about it.
https://user-images.githubusercontent.com/19151258/66028917-ca69b300-e506-11e9-984e-432cead8d197.gif

I reworked reorder API and it no longer reflects initial description. New API:

    // Reordering
    // - Call after each item that can be reordered.
    // - Returns -1/+1 if item should be swapped with the previous/next one, or 0 for no action.
    // - Only items within current or specified id scope participate in item reorder.
    // FIXME: "bool vertical" should be replaced with "ImGuiAxis axis", but ImGuiAxis is in imgui_internal.h
    IMGUI_API int           ItemReorder(ImGuiID id, bool vertical);
    IMGUI_API int           ItemReorder(const char* id, bool vertical);
    IMGUI_API int           ItemReorder(bool vertical);

And usage (error checking is ignored for brevity):

for (int i = 0; i < 10; i++)
{
    ImGui::Button(items[i]);
    if (int d = ImGui::ItemReorder(true))
        ImSwap(items[i], items[i + d]);
}

New API closely mimics behavior of "Drag to reorder items (simple)" in the demo, however it also solves several important issues:

  • Gaps between reorderable items can exist and they do not create a reorder feedback loop.
  • Reorder only happens when reorderable item is dragged over another reorderable item.
  • Multiple distinct reorderable lists may exist side by side and they do not interfere with each other (nothing happens if reorderable item from list A is dragged on to reorderable item in list B).

From the point of view of UI user new API is much more intuitive and comfortable to use. We no longer must precisely aim at a spot between two items that is couple pixels high. Instead reorder happens as soon as mouse cursor reaches another reorderable item.

Complicated reordering as displayed in this gif is no longer possible, and that is a good thing. A sensible approach to achieve both reordering and reparenting would be mixing DragAndDrop and Reorder APIs. Reorderable item may show a button for reordering and may use a DragAndDrop API for reparenting.
reorder

Old code is archived in https://github.com/rokups/imgui/tree/reorder-api-dnd

@ocornut ocornut added the drag drop drag and drop label Oct 2, 2019
int prev_i = items.index_from_ptr(items.find(color));
items.find_erase(color);
items.insert(items.begin() + i + (prev_i > i ? 1 : 0), color);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggestion: I'd aim to move the block code above in the loop, maybe start the loop with -1 or end with size()+1 and skip displaying the item but always display the reorder target.

Copy link
Owner

Choose a reason for hiding this comment

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

Would be nice is the demo also implemented the other form of dragging (which doesn't use the drag and drop api)

imgui.h Outdated
ImGuiDragDropFlags_AcceptPeekOnly = ImGuiDragDropFlags_AcceptBeforeDelivery | ImGuiDragDropFlags_AcceptNoDrawDefaultRect // For peeking ahead and inspecting the payload before delivery.
ImGuiDragDropFlags_AcceptPeekOnly = ImGuiDragDropFlags_AcceptBeforeDelivery | ImGuiDragDropFlags_AcceptNoDrawDefaultRect, // For peeking ahead and inspecting the payload before delivery.
ImGuiDragDropFlags_AcceptNoBorderPadding = 1 << 13, // Disable extra padding of border rendered around drop destination.
ImGuiDragDropFlags_ReorderHorizontal = 1 << 20, // List that is to be reordered is horizontal.
Copy link
Owner

Choose a reason for hiding this comment

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

1 << 20 should be 1 << 14

imgui.h Outdated
@@ -897,7 +898,9 @@ enum ImGuiDragDropFlags_
ImGuiDragDropFlags_AcceptBeforeDelivery = 1 << 10, // AcceptDragDropPayload() will returns true even before the mouse button is released. You can then call IsDelivery() to test if the payload needs to be delivered.
ImGuiDragDropFlags_AcceptNoDrawDefaultRect = 1 << 11, // Do not draw the default highlight rectangle when hovering over target.
ImGuiDragDropFlags_AcceptNoPreviewTooltip = 1 << 12, // Request hiding the BeginDragDropSource tooltip from the BeginDragDropTarget site.
ImGuiDragDropFlags_AcceptPeekOnly = ImGuiDragDropFlags_AcceptBeforeDelivery | ImGuiDragDropFlags_AcceptNoDrawDefaultRect // For peeking ahead and inspecting the payload before delivery.
ImGuiDragDropFlags_AcceptPeekOnly = ImGuiDragDropFlags_AcceptBeforeDelivery | ImGuiDragDropFlags_AcceptNoDrawDefaultRect, // For peeking ahead and inspecting the payload before delivery.
ImGuiDragDropFlags_AcceptNoBorderPadding = 1 << 13, // Disable extra padding of border rendered around drop destination.
Copy link
Owner

Choose a reason for hiding this comment

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

Inconsistent naming between _AcceptNoBorderPadding vs style.AcceptDropPadding ?

I think Border could be renamed, is a reference to how we draw this drop target but we could imagine it would became a solid thin rectangle, so we should focus on semantic e.g. _AcceptNoPadding

And style.DragDropAcceptPadding.
But I'm not sure we need this style variable at all for now unless you have a clear use for it.

bb.Max.x += GetContentRegionAvail().x;
bb.Min.y -= g.Style.ItemSpacing.y;
}

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure I understand the logic there, why is g.FontSize used? Is that assuming a certain size for items?
For trees with indentation we'll also want way for the drop target so cover whole width (like ImGuiTreeNodeFlags_SpanFullWidth) so maybe we need more use of this api before designing the thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why is g.FontSize used

I intended to get a line height here. Is there a better way? Admittedly font sizes are bit confusing to me, lots of different variables that affect font size.

Copy link
Owner

Choose a reason for hiding this comment

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

Why a "line height" for a separator between item? Shouldn't the separator be very thin (roughly ~style.ItemSpacing.y?).

Text-only line are g.FontSize high, most lines with framed widgets are g.FontSize + style.FramePadding.y * 2

Copy link
Owner

@ocornut ocornut Oct 2, 2019

Choose a reason for hiding this comment

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

Ah I think you want to use window->DC.PrevLineSize.y here?

@ocornut
Copy link
Owner

ocornut commented Oct 2, 2019

Thank you @rokups.
I have posted a couple of comments. I made in the code/commit itself as an experiment in reviewing but I'm thinking perhaps proper paragraph would be more helpful. (let me know!)

@ocornut ocornut changed the title Reorder api Drag and Drop Reorder api Oct 2, 2019
@ocornut ocornut changed the title Drag and Drop Reorder api Drag and Drop helper "reorder drop target" api Oct 2, 2019
@rokups
Copy link
Contributor Author

rokups commented Oct 2, 2019

I made in the code/commit itself as an experiment in reviewing but I'm thinking perhaps proper paragraph would be more helpful.

In-code comments are awesome, please keep doing that 👍

@rokups
Copy link
Contributor Author

rokups commented Oct 7, 2019

I was thinking and thinking and thinking about this and came to a conclusion that this PR is no good. Proposed helper API implements a terrible UX for reordering. My main issues:

  1. Inserted drag area between items is pretty small. Sometimes it takes trying to drop item on to it.
  2. In case of trees multiple drop areas overlap each other at the boundaries where tree levels meet. You can observe it somewhere in the following gif. Moving mouse horizontally makes drop area longer or shorter on horizontal axis. Longer area belongs to a parent while shorter one to the last child of that parent.
    reorder

As a possible solution i was considering using simple reordering using mouse drag delta to reorder children within a single parent and use drag&drop api for reparenting items to arbitrary parents. That would require something like holding down a modifier key to enable drag source so that drag delta reodering does not break.

What do you think? There surely must be a better way.

@ocornut
Copy link
Owner

ocornut commented Oct 7, 2019

Hello Rokas,

(1)
Attached a patch on top of your branch (you may want to apply on your side)
patch.zip

(2)
I will merge the "simple" reorder demo asap as well as switching to TreeNode instead of Bullet+Indent, so the branch will have minor fixes to apply.

(3)
On the topic you raise on your last post.
I think the demo is really promising. I agree with the issue you mention but that's the standard behavior used by many other applications, so I think we should just aim to improve it.

  • "Inserted drag area between items is pretty small" perhaps the reorder target could be taller, overlapping previous and subsequent item. Note that in BeginDragDropTargetCustom() we already use the target surface derived from DragDropTargetRect to solve overlapping items and giving priority to the smaller surface, or it may just work as is. (Might need a more explicit flag to not assume the fact that other target will be bigger, I don't know).

  • Minor: Dragged element may need some feedback at the source location (there's an internal field ImGuiSelectableFlags_DrawHoveredWhenHeld which I've been only using for header elements in the Tables branch, which might become a default behavior, or a public flag for Selectable?)

  • Minor: the reorder target perhaps should just be a thick line instead of a rectangle?

  • We probably need to come with a mechanism to signify that a drag is not allowed. Altering hovering state of underlying widget? Altering mouse cursor? Altering tooltip?
    That same mechanism could be used when e.g. dragging in the concealed area behind a modal window. GLFW will expose a "not allowed" cursor (currently missing, see Additional Cursors glfw/glfw#427) which may help but we could design a fallback for it.

@ocornut
Copy link
Owner

ocornut commented Oct 7, 2019

I have now merged the "simple reordering" part of the demo, the one not actually using the Drag and Drop api, so your PR will conflict, should be an easy rebase+fix+force push hopefully!

@rokups
Copy link
Contributor Author

rokups commented Oct 8, 2019

perhaps the reorder target could be taller, overlapping previous and subsequent item.
the reorder target perhaps should just be a thick line instead of a rectangle?

These are very good ideas!

One last thing. What do we do about this bit?
reorder

Dragged element may need some feedback at the source location

Maybe this feedback could be an indicator were user is parenting a node?

We probably need to come with a mechanism to signify that a drag is not allowed.

Do we? I mean it is the default state. 99% widgets are not draggable.

@AnimatorJeroen
Copy link

Hi ocornut and rokups. This PR looks really nice. Do you have an idea when it will be committed? I tried to replace the 3 modified files: imgui.cpp, imgui.h and imgui_demo.cpp to use drag and drop in my personal project with imgui v1.74, but I get errors with imgui_draw.cpp: "Out-of-line definition of 'CalcCustomRectUV' does not match any declaration in 'ImFontAtlas' ". Thanks in advance, Jeroen

@rokups
Copy link
Contributor Author

rokups commented Jan 21, 2020

Hey @AnimatorJeroen. This is more of a proof of concept. I am not sure it is a right way to do reordering. No idea if we are going to rework it and merge it. I rebased this branch on to master without any issues and no changes were required. Maybe you could provide full error messages?

@AnimatorJeroen
Copy link

Hi Rokups, sorry for the late reply. I use v 1.75 WIP now but I guess I am doing something wrong here. When I replace imgui.cpp, imgui.h and imgui_demo.cpp with your modified versions, I get 17 errors. Thank you!

1>imgui.cpp
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(3887,24): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4125,24): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4126,24): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4132,51): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4136,5): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(4137,39): error C2039: 'WindowsSortBuffer': is not a member of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui_internal.h(989): message : see declaration of 'ImGuiContext'
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(7364,1): error C2244: 'BeginTooltipEx': unable to match function definition to an existing declaration
1>C:\dev\windows animation app prototype\animation app prototype\vendor\imgui\imgui.cpp(7363): message : see declaration of 'BeginTooltipEx'

@rokups
Copy link
Contributor Author

rokups commented Feb 5, 2020

You should not replace files like that. You should either use all files from my branch or apply changes from this PR manually. Replacing half of library with older versions leads to incompatibilities as you can see.

@rokups
Copy link
Contributor Author

rokups commented Oct 11, 2021

I updated first post detailing rework pushed to the branch of this PR. Rework is quite different from initially proposed code so please read first post again. Previous code has been archived in another branch.

People following #143 may find this code useful.

@ocornut ocornut force-pushed the master branch 2 times, most recently from 8b83e0a to d735066 Compare December 13, 2021 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
drag drop drag and drop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants