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

selection of entities for dragging is too greedy #161

Closed
whitequark opened this issue Jan 17, 2017 · 11 comments · Fixed by #672
Closed

selection of entities for dragging is too greedy #161

whitequark opened this issue Jan 17, 2017 · 11 comments · Fixed by #672
Labels
Milestone

Comments

@whitequark
Copy link
Contributor

Open the attached file:
bug.zip
Rotate to the following view and position the cursor as follows:
2820
Drag with mouse. Both points will get dragged.

As a bonus I once got this crash:

File /home/whitequark/Work/solvespace/src/dsc.h, line 334, function FindById:
Assertion 'Cannot find handle' failed: ((t != NULL) == false).
Backtrace:
 0: ./bin/solvespace(_ZN10SolveSpace14assert_failureEPKcjS1_S1_S1_+0x78) [0x55580e551107]
 1: ./bin/solvespace(_ZN10SolveSpace6IdListINS_6EntityENS_7hEntityEE8FindByIdES2_+0x5a) [0x55580e51ad5c]
 2: ./bin/solvespace(_ZN10SolveSpace6Sketch9GetEntityENS_7hEntityE+0x24) [0x55580e51a42e]
 3: ./bin/solvespace(_ZN10SolveSpace14GraphicsWindow21StartDraggingByEntityENS_7hEntityE+0x20) [0x55580e5d335e]
 4: ./bin/solvespace(_ZN10SolveSpace14GraphicsWindow24StartDraggingBySelectionEv+0xa0) [0x55580e5d34e6]
 5: ./bin/solvespace(_ZN10SolveSpace14GraphicsWindow10MouseMovedEddbbbbb+0xa87) [0x55580e5d3f71]
 6: ./bin/solvespace(_ZN10SolveSpace14GraphicsWidget22on_motion_notify_eventEP15_GdkEventMotion+0xd0) [0x55580e52ec22]
...

but have no real way to reproduce it except by randomly dragging.

@whitequark whitequark added the bug label Jan 17, 2017
@whitequark whitequark added this to the 3.0 milestone Jan 17, 2017
Evil-Spirit added a commit to Evil-Spirit/solvespace-master that referenced this issue Jan 20, 2017
@Evil-Spirit
Copy link
Collaborator

@whitequark, updated here

@whitequark
Copy link
Contributor Author

@Evil-Spirit But not the first bug, no?

@Evil-Spirit
Copy link
Collaborator

@whitequark, not yet.

@whitequark
Copy link
Contributor Author

@Evil-Spirit Another bug with dragging:
open bug2.zip select g00a then try to drag the circle around by its center. This worked exactly once for me, and then stopped.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Jan 27, 2017

@whitequark, Unfortunately I can't open this file (packed filesize is zero). Could you reattach please?

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Jan 30, 2017

@whitequark,
I have thought a lot and perform some experiments.
There are seriuos issues with the current selection/hovering logic as it described in https://github.com/whitequark/solvespace/issues/213

when user clicks on the screen where there are many overlapping entities, we actually remember two entities:

  • Entity in the current workplane if any, or from the last group in order otherwise. This is what goes into the "group selection" thing, and is otherwise operated on.
  • Entity from a request. This will be used when dragging. This way, we can drag e.g. a sketch on which an extrusion group is based, even if the extrusion group is active and overlaps the sketch.
  1. We can't use isFromRequest criteria for prioritizing because we can (and want) drag non-request entities: extruded, step-n-tf, etc.
  2. We should drag exactly what we are hovering (or exactly equal entity). Not doing like this is reason for if a constraint is selected, prioritize it for starting a drag #174
  3. We can't use distance criteria in safe way to decide about entities equality (because distance is depend on entity style width which can be different for source and copied entities)
  4. We have to direct-prioritize point hovering/selection (we can't use zIndex criteria or should do it the different for rendering and selection canvases)
  5. I have no idea about how selecting/dragging from last group can save situation

@whitequark
Copy link
Contributor Author

@Evil-Spirit OK, I'll think more about it too.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit@3c8cc2e

@phkahler
Copy link
Member

The logic to fix this might be something like:
When selecting for dragging:
First find the item that normal selection would use - A
Then while looking for items in previous groups
Only use from a previous group IF the item A is not "noticeably" closer to the mouse.

The challenge is "noticeably closer". Dragging items from an underlying sketch is great but often involves something directly under something in the current group. We can restructure my proposed fix for #174 to allow this but the criteria for "noticeably" will need some help/thought.

@phkahler
Copy link
Member

I created an issue161 branch off my issue174 branch which almost includes a fix for this on top of the other ones. There is a line which needs the criteria for prioritizing the item that selection would use over an item from a previous group. Right now that line reads:

bool prioritizeStatic = false;

If you can define "static item is obviously closer to the mouse than the other one" and drop that condition in that line it should fix this issue while leaving the previous fix intact.

Remember, dragging uses the same picking logic as selection except that it prioritizes items from previous groups. But to fix this we need an exception to the exception in order to prioritize the normal selection. If the two points are on top of each other we want to prioritize the one from a prior group for dragging. This is splitting hairs.

This really is a corner case ;-)
branch: https://github.com/phkahler/solvespace/tree/issue161
Key function sans magic condition:

// This is different from ChooseFromHoverToSelect() by allowing us to drag
// an entity from a previous group. However it doesn't behave quite the
// same in the current group, so if it fails at selecting a underlying
// entity we shall use ChooseFromHoverToSelect instead
GraphicsWindow::Selection GraphicsWindow::ChooseFromHoverToDrag() {
    Selection staticSelect = ChooseFromHoverToSelect();

    Selection sel = {};
    Group *activeGroup = SK.GetGroup(SS.GW.activeGroup);

    for(const Hover &hov : hoverList) {
        if(hov.selection.entity.v == 0) continue;
        if(!hov.selection.entity.isFromRequest()) continue;
        // determine the group of hov.selection
        hGroup hg = {};
        if(hov.selection.entity.v != 0) {
            hg = SK.GetEntity(hov.selection.entity)->group;
        } else if(hov.selection.constraint.v != 0) {
            hg = SK.GetConstraint(hov.selection.constraint)->group;
        }
        Group *g = SK.GetGroup(hg);
        bool prioritizeStatic = false;
        if((g->order < activeGroup->order) && (g->order > 0) && !prioritizeStatic) {
            sel = hov.selection;
            break;
        }
    }
    if(!sel.IsEmpty()) {
        return sel;
    }
    return staticSelect;
}

@phkahler
Copy link
Member

I have a feeling the issue here is much deeper. The goal is not so much to prioritize move entities in earlier groups is it? The normal selection logic allows picking things from any group. Isn't the real goal something like "move the earliest (in group order) item that is linked to and will end up moving the thing I'm hovering over"? That is a very complex thing to get right and the code to do it thus far is very simplistic, though I think with the 174 fix it's rather effective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants