Page MenuHome

Fix T64585: Mouse jumps to bottom when moving the playhead.
ClosedPublic

Authored by George Vogiatzis (Gvgeo) on May 15 2019, 11:50 AM.

Details

Summary

Add separate Wrap(grab mouse) for X and Y axis
Use wrap X axis when moving the playhead.
Also add drag in X axis for header, footer.

Diff Detail

Event Timeline

Not exactly sure how much should change.
I left GHOST_EventCursor *GHOST_SystemWin32::processCursorEvent basically untouched.
And there are 3 more calls to WM_cursor_grab_enable with wrap bool instead of int now.
Ofc it is possible to set Y bounds -/+ 10000 instead of making all of these changes.

William Reynish (billreynish) requested changes to this revision.May 15 2019, 4:35 PM
  • This breaks continuous grab in all directions here, not just Y
  • This breaks continuous grab for moving keyframes, not just scrubbing
This revision now requires changes to proceed.May 15 2019, 4:35 PM
Brecht Van Lommel (brecht) requested changes to this revision.May 15 2019, 5:08 PM

This code only includes the changes for Windows, it will break on macOS and Linux.

source/blender/windowmanager/intern/wm_event_system.c
1456

Can we assign GHOST_kGrab* values to this directly, instead of numbers?

Campbell Barton (campbellbarton) requested changes to this revision.May 16 2019, 6:58 AM
Campbell Barton (campbellbarton) added inline comments.
intern/ghost/GHOST_Types.h
375–377

Prefer this is not categorized as a different kind of grab, because most logic for grabbing applies no matter which axis is grabbed.
You have to disable grab after & there is a virtual location thats calculated.

Instead there can be X/Y grab axis booleans.

George Vogiatzis (Gvgeo) edited the summary of this revision. (Show Details)EditedMay 16 2019, 11:08 AM
George Vogiatzis (Gvgeo) updated this revision to Diff 15373.

Add changes for other systems.
Use bool to disable Y axis.
Add X wrap for headers.

@George Vogiatzis (Gvgeo) Here building fails with this patch. Does it apply to master?

George Vogiatzis (Gvgeo) updated this revision to Diff 15479.EditedMay 20 2019, 4:22 PM

Fix mistake for macOS version.

Tested:

This works better for scrubbing the playhead no doubt, but this affects *everything*. Moving curve points outside the view in the Graph Editor, for example doesn't work like before - you can't continue outside of the Y bounds.

This change should ideally only affect scrubbing.

However, if that is not possible, I do see this as probably a better default, but again, ideally it should only affect scrubbing.

Brecht Van Lommel (brecht) requested changes to this revision.May 21 2019, 9:27 AM
Brecht Van Lommel (brecht) added inline comments.
source/blender/windowmanager/intern/wm_event_system.c
1476–1481

This can't be decided by editor type, it really must be per operator.

This revision now requires changes to proceed.May 21 2019, 9:27 AM
George Vogiatzis (Gvgeo) updated this revision to Diff 15490.

Affect only when moving the playhead.

Tested - seems to work well.

Personally I'm wondering why x should be a special case here. Why not have an enum with CURSOR_WRAP_X, CURSOR_WRAP_Y and CURSOR_WRAP_XY and pass it to WM_cursor_grab_enable. That seems much cleaner to me.

Brecht Van Lommel (brecht) requested changes to this revision.May 21 2019, 1:14 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/windowmanager/intern/wm_event_system.c
1477–1479

This isn't quite right either. It should be defined on the operator itself, not with specific hardcoded exceptions.

You'd have e.g. an OPTYPE_GRAB_CURSOR_X instead of OPTYPE_GRAB_CURSOR.

This revision now requires changes to proceed.May 21 2019, 1:14 PM

Personally I'm wondering why x should be a special case here. Why not have an enum with CURSOR_WRAP_X, CURSOR_WRAP_Y and CURSOR_WRAP_XY and pass it to WM_cursor_grab_enable. That seems much cleaner to me.

I agree with this, the separate boolean makes it less clear to me.

Passing booleans is confusing, in the ghost code though it's handling a single x-axis doesn't need to be a special case.

Instead of...

if (grab_xy) {
  ... handle xy ...
} else if (grab_x) {
  ... handle x ...
}
else if (grab_y) {
  ... handle y ... (not done in this patch, but that's what would be done to extend the current method).
}

When we could do:

if (grabbing) {
  if (grab_x) {
    ... handle x ...
  }
  if (grab_y) {
    ... handle y ...
  }
}
George Vogiatzis (Gvgeo) updated this revision to Diff 15527.

Changed everything based on your latest requests.

  • Add RNA enum (so Python operators can use this).
  • Rename axis flag (no need for this to be cursor specific)

Looks good to me. @Campbell Barton (campbellbarton), not sure if you were waiting for something to commit this?

This revision is now accepted and ready to land.May 28 2019, 3:09 PM