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

Globe - basic infrastructure, raster layer adaptation for globe #3783

Merged
merged 53 commits into from Mar 15, 2024

Conversation

kubapelc
Copy link
Contributor

@kubapelc kubapelc commented Mar 4, 2024

This is the first PR of the implementation of vector globe view for MapLibre. It is a selection of changes from my main vector globe branch as well as interface and changes from @Pheonor's globe implementation. More PRs will follow with more of the globe rendering implementation. I plan to split PRs into smaller ones for easier reviewing, as requested.

Issue & discussion of globe:

maplibre/maplibre#190
maplibre/maplibre#161

Additions and changes

  • base classes and interface for globe, a unification of both my and @Pheonor's forks, so that we can both do PRs independently in the future
  • changes to map.ts and Map constructor to include projection
  • system for projection-dependent vertex shader prelude code
    • such code is injected at the beginning of every vertex shader
    • see src/shaders/_projection_mercator.vertex.glsl for an example
    • example: function projectTile() projects a point in local tile coordinates (0..EXTENT) to screen
      • for mercator, this is mostly equivalent to a simple matrix multiplication (usually replaces u_matrix * vec4(a_pos, 0.0, 1.0) in shaders)
      • for globe, this does a projection from mercator to a 3D point on the surface of a sphere, which is then transformed with a projection matrix
      • additionally, for globe projection, this function automatically handles animated transition from globe to mercator (for high zoom levels)
    • this system should (in theory) allow for simpler implementation of different projections in the future
  • ProjectionBase class, and its implementations for Mercator and Globe projections
    • note that globe projection uses an instance of mercator projection internally, since globe transitions to mercator at high zoom levels
    • the currently used projection class is exposed in map.ts to allow custom layers to implement globe view using shader projection preludes
    • the projection class will likely expand in the future once proper controls and other non rendering-related features are implemented for globe
  • adaptation of the raster layer for globe (other layers are still mercator)
    • raster layer is a relatively simple example of how the new projection classes and projection shader preludes are used
  • an example webpage with globe view (test/examples/globe.html)
  • some minor changes
    • additional comments
    • typo fixes
    • simplify tilesAfterTime implementation in terrain_source_cache.ts

Example

A screenshot of a globe with a satellite map

See test/examples/globe.html for a web page that displays a satellite map projected onto a globe. Note that only the raster layer is currently adapted for globe projection, so lines and symbols will still display as if on a mercator map.

The projection will automatically transition to mercator at high zoom levels. The transition is animated and happens at a point when globe is already almost indistinguishable from mercator projection, so it is nearly unnoticeable.

Changes compared to Pheonor's repo

@Pheonor please also review this PR, especially the following changes and adaptations from your branch:

  • I've excluded the project and unproject methods from the projection classes for now, as they are not used by anything (yet).
  • I've moved the place where projection is stored from transform.ts into map.ts, as transform might exist in multiple instances, but map should (as far as I know) only ever exist in one single instance.
  • The globe example webpage is from your branch, just slightly modified (added a title and description).

Problem: where to place granularity settings?

Granularity determines how much subdivision is applied to tile geometry. For example, a subdivision granularity of 4 will "split" all geometry in a tile as if it was cut by the edges of a 4x4 grid. Different zoom levels have different granularities. Granularity for a given zoom level is determined by a simple function.

It is relatively clear how to set granularity levels for globe projection, and thus, if MapLibre only ever used globe view, those values could be static. But what if the user wants a map that will always use mercator? Then there is no need for subdivision to slow down tile parsing and rendering. Or what if a different projection in the future needs different granularities?

Subdivision functions are used in *_bucket.ts (fill, fill-extrusion and line buckets), right before generating vertex buffers. The bucket functions run in a separate web worker which has no access to the map or transform instance, as far as I know. Furthermore, the granularity doesn't depend only on zoom level, but also on what type of geometry is getting subdivided - for example line layers use higher granularity than fill layers.

Currently, granularity settings are static, both in the sense that they cannot change eg. with projection and that they use the static keyword to be accessible from anywhere.

I have no idea how to solve this. The data about which tiles need to be loaded (data which ultimately ends up in the bucket) passes through complex machinery and it is hard for me to even navigate through it.

In this PR, subdivision is only used for stencil masks, but following PRs will utilize it for fill, fill extrusion and line geometry as well.

Failing tests & other issues

As far as I know, about 300 render tests fail in my branch. After consulting with @wipfli I decided to publish the PR anyway, but I will continue debugging the tests and I'll try to find a fix. I don't know about the state of unit tests, I haven't managed to even run those yet. But using this branch of MapLibre for displaying a map in a web browser should work without any issues.

On Chrome, you may notice a warning about a GPU buffer guarded by a FenceSync object being read before the fence is signaled. I know about this issue and will attempt to fix it. It is related to globe projection (see class ProjectionErrorMeasurement in src/geo/projection/globe.ts). This is now fixed.

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.
    • Completed, as long as doc comments are enough.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 83.10000% with 169 lines in your changes are missing coverage. Please review.

Project coverage is 86.35%. Comparing base (adc7f17) to head (4526c16).
Report is 5 commits behind head on globe.

Files Patch % Lines
src/geo/projection/globe.ts 70.29% 88 Missing and 13 partials ⚠️
src/geo/projection/mercator.ts 77.55% 30 Missing and 3 partials ⚠️
...o/projection/globe_projection_error_measurement.ts 88.69% 15 Missing and 4 partials ⚠️
src/render/painter.ts 85.10% 12 Missing and 2 partials ⚠️
src/render/draw_collision_debug.ts 50.00% 0 Missing and 1 partial ⚠️
src/ui/map.ts 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            globe    #3783      +/-   ##
==========================================
- Coverage   86.56%   86.35%   -0.22%     
==========================================
  Files         240      247       +7     
  Lines       32273    33098     +825     
  Branches     1956     2024      +68     
==========================================
+ Hits        27937    28581     +644     
- Misses       3416     3560     +144     
- Partials      920      957      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Member

HarelM commented Mar 4, 2024

Thanks a lot for opening this PR!!
It will take me some time to review it as it has a lot of changes.
I'll start with some basic stuff and then later on move to the more in depth, so please bare with me.
Also note that you can use this PR to see the results of the render tests.
If your code is "opt in" then the render tests should not fail, in theory, but everything is possible.
If you have any issue running the tests or understanding what they do, let me know and I'll do my best to help.
It seems that the unit tests passed, so this is a good sign.

@kubapelc
Copy link
Contributor Author

kubapelc commented Mar 5, 2024

I fixed the issue with Chrome warning related to FenceSync and pixel readback.

@HarelM
Copy link
Member

HarelM commented Mar 7, 2024

@kubapelc do you think you can create a PR only for the Mesh class addition and changes? (should be like 4-5 files I think).
I want to push only this and try and use this Mesh class in the sky maybe?
I can try and do it if you find it complicated...

@HarelM
Copy link
Member

HarelM commented Mar 7, 2024

For now I copied the mesh.ts file to the sky2 branch, I hope it won't cause a merge issue once sky is merge...

src/ui/map.ts Outdated Show resolved Hide resolved
src/ui/map.ts Outdated Show resolved Hide resolved
src/ui/map.ts Outdated Show resolved Hide resolved
src/source/source_cache.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Member

HarelM commented Mar 14, 2024

I think I resolved all the current implemented review comments, I left those that are still relevant I believe.
I've opened a design proposal for the style spec:
maplibre/maplibre-style-spec#568
We might be able to ignore it for now like we do for the zoom and pitch that can be set for both the map configuration and the style, so it's not a blocker I guess, make sure to check my assumption.

@HarelM
Copy link
Member

HarelM commented Mar 14, 2024

I think most code review comments were resolved.
Please update the build test to have the updated bundle size so that all tests pass.
I'm wondering if more tests are needed or should we add them after more code changes have been made, as currently there are unit tests missing for the new classes, but this can wait for later PRs.

src/geo/projection/globe.ts Outdated Show resolved Hide resolved
src/render/painter.ts Outdated Show resolved Hide resolved
src/geo/projection/globe.ts Outdated Show resolved Hide resolved
src/ui/map.ts Outdated Show resolved Hide resolved
@kubapelc
Copy link
Contributor Author

@HarelM I've added some render tests in a previous commit, and now I've added unit tests for globe and mercator as well, although they don't test too much functionality. I'll think about what else I can cover with unit tests.

I've fixed the bundle size in build test.

@HarelM
Copy link
Member

HarelM commented Mar 14, 2024

Great, seems like a recent change caused tests to fail.
The added unit tests are great, thanks!
I think we are good from tests perspective, once they pass :-)
I don't have other comments ATM.

@HarelM HarelM merged commit 6132078 into maplibre:globe Mar 15, 2024
13 checks passed
@HarelM
Copy link
Member

HarelM commented Mar 15, 2024

This is now merged to globe branch, I'll do my best to keep this brach up-to-date with main.
Please continue pushing improvements to it.

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.

None yet

4 participants