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

Add transformCameraUpdate callback #2535

Merged
merged 8 commits into from
May 17, 2023

Conversation

Pessimistress
Copy link
Contributor

@Pessimistress Pessimistress commented May 16, 2023

For #1545

Change List

  • Add transformCameraUpdate option to Map
  • Inject _startCameraUpdate and _endCameraUpdate into the following methods where Camera.transform is modified:
    • Camera.jumpTo
    • Camera.easeTo
    • Camera.flyTo
    • HandlerManager._updateMapTransform
      See inline comments
  • A helper class HandlerBase for handlers to access the correct camera state

There should be no behavioral change or performance overhead if transformCameraUpdate is not assigned.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

src/ui/camera.ts Outdated Show resolved Hide resolved
src/ui/camera.ts Outdated Show resolved Hide resolved
src/ui/camera.ts Outdated Show resolved Hide resolved
@birkskyum birkskyum marked this pull request as ready for review May 16, 2023 08:41
@birkskyum
Copy link
Member

birkskyum commented May 16, 2023

To see how this PR worked in practice, I made a debug page here (it can be dumped next to the other, if this PR branch is checked out):
https://github.com/birkskyum/maplibre-gl-js/blob/transform-camera-update/test/debug-pages/reactive.html

I think it works as expected, with minimal change to our public API, and if I'm not mistaken it doesn't see to contain any breaking changes. Here is a small clip of the left map driving the right, without constantly lagging a frame behind.

Screen.Recording.2023-05-16.at.12.40.03.mov

src/ui/map.ts Outdated Show resolved Hide resolved
birkskyum
birkskyum previously approved these changes May 16, 2023
@birkskyum birkskyum self-requested a review May 16, 2023 10:59
@birkskyum birkskyum dismissed their stale review May 16, 2023 11:00

mis click, some naming is needed

@birkskyum
Copy link
Member

birkskyum commented May 16, 2023

This little hook has surprisingly great utility - should not be overlooked.

  • Without a js framework (like the debug-pages i made) it's possible to use gl js in a reactive way.
  • All JS frameworks (react, vue, svelte, solid, angular etc.), especially where a wrapper isn't available, will get better ergonomics. You can finally get the direct camera values synced to a store, and they are not lagging 1 frame behind - terrain makes this even more important when the camera 'jumps' over a peak as that camera value wasn't possible to get at all.
  • Advanced wrappers (the list is growing) like react-map-gl, ngx-maplibre-gl, deckgl, maptiler-sdk-js, will have a much easier time dealing with the camera.
    @wipfli , fyi

src/ui/camera.ts Outdated Show resolved Hide resolved
src/geo/transform.ts Outdated Show resolved Hide resolved
src/ui/map.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented May 16, 2023

@Pessimistress thanks for taking the time to open this PR!
I think there are still some naming and small fixes left to do, but this looks great in general.

@Pessimistress
Copy link
Contributor Author

Some renames according to review comments:

  • Transform.copy(that: Transform) -> Transform.apply(that: Transform)
  • Camera._transformCameraUpdate -> Camera.transformCameraUpdate
  • Camera._transformInProgress; -> Camera._requestedCameraState
  • Camera._startCameraUpdate(): Transform -> Camera._getTransformForUpdate(): Transform
  • Camera._endCameraUpdate(tr: Transform) -> Camera._applyUpdatedTransform(tr: Transform)

I don't think transformCameraUpdate is necessarily confusing because the internal term "transform" as a camera/viewport is not exposed to the end user. There's already an option transformRequest that does a similar thing (intercept arguments and modify them before use).

How concerned are you about the bundle size?

@HarelM
Copy link
Collaborator

HarelM commented May 17, 2023

How concerned are you about the bundle size?

Not really concerned. Please update the size in the test.
Overall looks good. THANKS!

@birkskyum
Copy link
Member

birkskyum commented May 17, 2023

Thanks for the renaming, i find this easier to reason about. We need a changelog entry.

@birkskyum birkskyum requested a review from HarelM May 17, 2023 17:51
@birkskyum birkskyum merged commit 7c35299 into maplibre:main May 17, 2023
@birkskyum
Copy link
Member

Great job!

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.

3 participants