maplibre-gl-js icon indicating copy to clipboard operation
maplibre-gl-js copied to clipboard

Globe - controls, animations & transform+projection changes

Open kubapelc opened this issue 1 year ago • 5 comments

Globe - transform+projection changes

I've removed all changes to camera controls and animations for easier reviewing (they will come later), but I'll keep the changelog as it was. The demo still includes controls changes, but if you compile from source, you won't have them.

Issue & discussion of globe:

https://github.com/maplibre/maplibre/issues/190 https://github.com/maplibre/maplibre/discussions/161

Changes

Controls for globe - I believe these should now feel natural, but here is a quick summary of how they work and what edge cases are handled:

  • camera can now look at (or nearly at) the poles at any zoom level
  • panning works in a way that is very similar to mercator, and converges to mercator controls at high zooms
    • this means that the pan speed of longitude is increased as the camera moves nearer to the poles
    • at low zooms however, longitude pan speed is reduced when near the poles to prevent the globe from spinning wildly
  • zooming will now correctly keep the location under the cursor in the same place, as it does in mercator
    • this behaviour is not used near the planet edges, as it would move the camera too quickly and eventually cause glitches - an approximation is seamlessly used instead when cursor is near the horizon
    • zooming now works even if cursor is not on the planet surface at all - in this case the map zooms to a location near the horizon that is closest to the cursor
    • additionaly when zooming in/out with the cursor being placed too far away from the planet, the map is prevented from panning too quickly

Camera animations and functions implemented for globe projection:

  • fitBounds - correctly computes zoom & center to fit a given box, respects to planet curvature & padding
  • flyTo
  • easeTo
  • jumpTo

Other changes:

  • Transform is now an abstract class, with specialized implementations for every projection
    • moved most functions from the Projection interface to the Transform class
  • Projection interface is now much simpler, and mostly used for things with are independent of the map's state
  • implemented all relevant projection and unprojection functions for globe
    • unprojecting a pixel that does not lie on the planet surface will instead return the location on the planet horizon that is nearest to the pixel in screenspace
  • removed posMatrix - a per-tile projection matrix that was specific to Mercator projection and was explicitly used and passed to shaders, stored in the Tile class, etc. Mercator now supplies this matrix through the same mechanism as globe (getProjectionData function).
  • further symbol refactor - simplified projection code, removed all mercator-only code paths - the Projection interface (or rather the now general Transform class) is used instead, handling both projections as needed
  • changing map projection will now reload all tiles

Zoom & apparentZoom

Under globe, the planet's size is automatically adjusted when latituded is changed so that the map's center always exactly matches a mercator map. For example, zoom level 0 at equator will result in a small planet, but near Greenland it would be much bigger (think of how oversized Greenland is under mercator projection). Changing projection for a given map center & zoom will however display the exact same location with the exact same size thanks to this behaviour (experiment with the globe toggle in the demo!) Under globe, panning and other map controls automatically counteract this effect to keep the planet size constant, unless the user actually wants to zoom in.

This weird relation of planet size to zoom level can have unexpected effects when programmatic animations are used. Imagine you want to fly from Greenland at zoom 4 to the equator at zoom 4 with globe projection. You would expect the planet size to remain constant, since the zoom levels are the same, but it would actually decrease a lot at the equator!

For this reason most animations have a new optional parameter in options: apparentZoom. This exists alongside the already present zoom paremeter and either can be used. It specifies target "planet size" in relation to the starting planet size. So if want to fly from Greenland to equator, starting map zoom is 4 and target apparentZoom is 4, the final planet size will be the same as the starting size! If we would instead specify apparentZoom: 5, the planet at the end of the animation would be twice as large as at the start, if apparentZoom: 3 it would be half size, etc. If globe is disabled, apparentZoom works exactly like the old zoom option.

What is left

Globe should now be mostly usable. Here are the things that don't yet work:

  • custom layer API for globe
  • 3D model API for globe
  • markers are not hidden/faded when behind planet horizon
  • queryRenderedFeatures
  • globe transform cannot be constrained at the moment - it is unclear how constraining should be implemented, since snapping the camera so that out of bounds regions aren't at all visible would probably feel very unnatural
  • terrain - I'm not sure I'm going to be implementing that though

Other bugs at the moment:

  • Markers do not respect terrain depth, even on mercator projection. Hence the failing unit test. I have no idea what caused this...
  • options.offset semantics for camera animations (flyTo, easeTo) differ across globe and mercator, and I think the mercator implementation is actually bugged? (At least it does something different that the docs claim it should.)

Demo link

Play around with new globe controls here.

Launch Checklist

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

kubapelc avatar Jun 28 '24 12:06 kubapelc

Codecov Report

Attention: Patch coverage is 90.55662% with 246 lines in your changes missing coverage. Please review.

Project coverage is 87.67%. Comparing base (7f23795) to head (639ed7f).

Files Patch % Lines
src/geo/projection/globe_transform.ts 78.26% 147 Missing and 23 partials :warning:
src/geo/projection/mercator_transform.ts 96.70% 13 Missing and 9 partials :warning:
src/geo/transform_helper.ts 93.67% 11 Missing and 9 partials :warning:
src/geo/projection/projection_factory.ts 77.77% 6 Missing :warning:
src/util/util.ts 95.57% 0 Missing and 5 partials :warning:
src/geo/projection/globe.ts 89.65% 3 Missing :warning:
src/render/terrain.ts 0.00% 2 Missing and 1 partial :warning:
src/source/query_features.ts 57.14% 2 Missing and 1 partial :warning:
src/ui/camera.ts 94.33% 0 Missing and 3 partials :warning:
src/geo/projection/mercator.ts 66.66% 2 Missing :warning:
... and 8 more
Additional details and impacted files
@@            Coverage Diff             @@
##            globe    #4341      +/-   ##
==========================================
+ Coverage   87.43%   87.67%   +0.24%     
==========================================
  Files         254      258       +4     
  Lines       35342    36291     +949     
  Branches     2331     2356      +25     
==========================================
+ Hits        30900    31819     +919     
- Misses       3376     3400      +24     
- Partials     1066     1072       +6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jun 28 '24 12:06 codecov-commenter

I also observed a lot of quirks with options.offset. This feature seems to be very buggy. Also there are some unit tests that treat these bugs as the desired behavior. At least so it appears to me.

sbachinin avatar Jun 28 '24 13:06 sbachinin

This is a huge PR to review, is there a way to split it somehow to smaller features/ increments?

HarelM avatar Jun 29 '24 19:06 HarelM

I've removed all the modifications to map controls & camera animations, which should make this somewhat smaller. Now the "only" change in this PR is the projection+transform rewrite, which includes mostly small changes, but across many many files (and a huge change to transform.ts).

kubapelc avatar Jun 30 '24 08:06 kubapelc

The failing unit tests are related to marker's opacity changes when behind the terrain. It is probably related to some of the method name changes, and the fact that mocking them now isn't working as before. It should probably be an easy fix, I would look at the following method changes: map.transform.lngLatToCameraDepth and map.terrain.depthAtPoint

HarelM avatar Jun 30 '24 10:06 HarelM

I've merged the following PR to main and afterward to the globe branch:

  • #3136

I think this is what created the last conflict. We'll need to think how to move forward as changing the transform object might create harder merges in the near future, so we should aim at finalizing the globe effort soon to avoid grief.

In any case, I see that some tests are failing, let me know if you need help fixing those.

HarelM avatar Jul 04 '24 19:07 HarelM

The failing unit tests are related to marker's opacity changes when behind the terrain. It is probably related to some of the method name changes, and the fact that mocking them now isn't working as before.

Thanks for the pointers! That was indeed the problem. Mocking the map's starting transform instance no longer works, since it gets replaced once style is loaded, since style sets the map's projection.

kubapelc avatar Jul 05 '24 13:07 kubapelc

According to latest coverage report (which might have bugs unfortunately, so you might need to verify this locally) the following files are missing some coverage: the utils classes should be 100% covered or something very close to it, regarding globe transform, it would be great if we could bring it up to 90% or so. image

Other than that, I don't have other inputs, it's hard to review the transform changes due to how it moved and the ability to compare to what was there before, so I'm relying mostly on code coverage here...

HarelM avatar Jul 07 '24 12:07 HarelM

I've added many more tests now.

kubapelc avatar Jul 08 '24 11:07 kubapelc

Thanks, there are a few last comments that I need to check on my side, I'll report back once I do. Overall, I think this is mostly ready, I'll do a last review in the coming days.

HarelM avatar Jul 09 '24 13:07 HarelM

I think my review is mostly done. Thanks @kubapelc for you patience and changes, and for putting up with my annoying review comments. There are still a few open review comments in this PR, but I don't know if they are worth following through. I'll merge this into globe and you can decide if you want to follow them or not, if there's anything to do with what I wrote.

HarelM avatar Jul 12 '24 18:07 HarelM

Thank you @HarelM for the in-depth code review! I'll probably leave the last two commented snippets (projection instance reference and animation) as they now, I'm not sure what better to do.

kubapelc avatar Jul 15 '24 08:07 kubapelc