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

Support desktop (click and drag) rotation #1592

Merged
merged 5 commits into from
Jul 20, 2023
Merged

Support desktop (click and drag) rotation #1592

merged 5 commits into from
Jul 20, 2023

Conversation

JaffaKetchup
Copy link
Member

@JaffaKetchup JaffaKetchup commented Jul 18, 2023

Resolves #1568.

Thanks to @TesteurManiak for the infrastructure implementation and POC and @ibrierley for the rotation algorithm itself. Also thanks to https://stackoverflow.com/questions/48916517/javascript-click-and-drag-to-rotate and https://github.com/openlayers/openlayers/blob/7099f14bd2348491c1550f7b0be929e5da00d83d/src/ol/interaction/DragRotate.js#L65.

Still to do before this can be release ready:

  • Ensure this does not break other existing gestures
  • Allow for other keyboard triggers
  • Add InteractiveFlag.cursorRotate (redundant with 'Allow for other keyboard triggers')
  • Consider renaming InteractiveFlag.rotate for clarity (InteractiveFlag.cursorRotate will not be added)
  • Allow for customizing rotation degree scale (1 by default) (does not work correctly and can cause rotation jumps)

Also moved InteractionOptions.enableScrollWheel to InteractiveFlags.scrollWheelZoom with deprecation.

Co-Authored-By: Ian <3901173+ibrierley@users.noreply.github.com>
Co-Authored-By: Guillaume Roux <rouxguillaume8@gmail.com>
@JaffaKetchup JaffaKetchup changed the title Basic initial implementation of desktop rotation Support desktop (click and drag) rotation Jul 18, 2023
@ibrierley
Copy link
Collaborator

I only included the round function as that was in the original solution I was looking at. I never had chance to do any detailed digging into whether it causes any issues without it. I did ponder it at the time and couldn't really figure why it was necessary.

Moved `InteractionOptions.enableScrollWheel` to `InteractiveFlags.scrollWheelZoom` (with deprecation)
Improved documentation of `InteractiveFlags`
@JaffaKetchup JaffaKetchup marked this pull request as ready for review July 19, 2023 09:53
Copy link
Collaborator

@TesteurManiak TesteurManiak left a comment

Choose a reason for hiding this comment

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

Just got some very minor remarks, otherwise LGTM! 👍

lib/src/map/options.dart Outdated Show resolved Hide resolved
lib/src/gestures/interactive_flag.dart Outdated Show resolved Hide resolved
@JaffaKetchup JaffaKetchup merged commit 9697c9d into master Jul 20, 2023
7 checks passed
@JaffaKetchup JaffaKetchup deleted the fix-1568 branch July 20, 2023 06:25
@androidseb
Copy link
Contributor

Thank you very much for adding in this feature! I've tested it just now on the demo page here:
https://demo.fleaflet.dev/

For me it works mostly fine, but I noticed the rotation jumps around a little bit in unexpected ways (maybe I don't have the correct expectations?)...

Here are some reproduction steps of the issue in case this helps:

  • Load the demo page here in the latest version of chrome (Version 116.0.5845.140 (Official Build) (64-bit)): https://demo.fleaflet.dev/
  • Hold the "Ctrl" keyboard key
  • With the left mouse button, drag the map from the top left corner down by just a few pixels
  • UX issue:
    • EXPECTED: the map rotates by just a few degrees
    • ACTUAL: the map is rotated instantly by 45 degrees, then by a few additional degrees

It appears wherever the mouse is dragged to relative to the center of the map view becomes the new north of the map. @JaffaKetchup Is this how it's expected to work? I would have expected the map rotation to allow us to "Drag" the map for rotation by a few degrees at once, not set the new north.

@JaffaKetchup
Copy link
Member Author

Hi @androidseb,
Indeed, this is the way it's supposed to function at the moment, but I do agree it might be better if we could just apply an offset to the current rotation, instead of setting the rotation outright. I'll see what I can do, but if I can remember correctly, trying to implement it like this caused issues.

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

Successfully merging this pull request may close these issues.

[FEATURE] Implement rotation functionality on mouse/cursor based platforms
4 participants