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 final PR #3963

Merged
merged 76 commits into from
Sep 30, 2024
Merged

Globe final PR #3963

merged 76 commits into from
Sep 30, 2024

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Apr 10, 2024

Launch Checklist

Resolves #307

Keeping this as draft right now, this PR will allow merging of the entire globe branch into main.

  • 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.

kubapelc and others added 7 commits March 15, 2024 13:32
* Port changes from main globe branch - basics

Fix minor issues so that it compiles.

* Fix PI redefinitions

* Fix stencil shader

* Port adaptation of raster layer for globe from main globe branch

* Add globe.html example from pheonor's repo

Minor changes (remove terrain, set initial zoom 0, change title and description)

* Better map projection parameter doc comment, warn when using unknown projection

* Mercator projectionData handles negative zoom correctly

* Comment clarification

* Fix spelling of "granularity"

* Add missing docs

* Convert ProjectionBase to an interface

* Do not leak GL object in globe projection error measurement, add a destroy method to projection

* Fix chrome performance warning, refactor error measurement

Warning fixed by changing ring buffer size to 1, making ring buffer pointless, so I removed it.

* Fix granularity capitalization

* Fix capitalization

* Fix typo

* Fix stencil mask triangle index order (this was causing failing render tests)

* Cleanup vertex shader projection interface

* Move projection creation function into its own file

* Remove getProjectionName

* Added comment for deduplicateWrapped

* Remove unused vertex-buffer-related code from image source

* Add globe raster layer render test

* More render tests - test transition to mercator

* Remove pointless test, add test descriptions

* Render test for rendering poles on globe

* SubdivisionGranularitySetting constructor takes an object

* Remove "defines" parameter from useProgram

* Refactor useProgram and Program constructor

* Properly format translatePosMatrix comment

* Refactor globe-specific code outside projection classes, remove stencil-specific granularity settings

* Refactor granularity settings to be more readable

* Minor refactor of ProjectionErrorMeasurement

* Refactor draw_raster.ts

* Move globe utility functions to utils.ts, use easeCubicInOut instead of smoothStep

* Simplify imports in globe.ts

* globe.ts refactor

* Move ProjectionErrorMeasurement to a separate file

* Refactor ProjectionErrorMeasurement

Change parseRGBA8float to a private static function, use isWebGL2 function instead of instanceof

* Refactor draw_raster.ts

* Refactor globe projection error measurement to not use Painter

* Painter.clearStencil creates custom ProjectionData instead of calling getProjectionData(null, null)

* Remove "deduplicateWrapped" functionality from source_cache.ts

* Globe projection no longer requires a map instance

* Painter doesn't pass `this` to `updateGPUdependent`

* isRenderingDirty is now a function

* Rename ProjectionBase to Projection

* Replace globeView property with setGlobeViewAllowed

* Add mercator and globe projection unit tests

* Remove tests that test for exact clipping planes

* Update build test with new bundle size

* isRenderingDirty is now a function
* Port changes from main globe branch - basics

Fix minor issues so that it compiles.

* Fix PI redefinitions

* Fix stencil shader

* Port adaptation of raster layer for globe from main globe branch

* Add globe.html example from pheonor's repo

Minor changes (remove terrain, set initial zoom 0, change title and description)

* Better map projection parameter doc comment, warn when using unknown projection

* Mercator projectionData handles negative zoom correctly

* Comment clarification

* Fix spelling of "granularity"

* Add missing docs

* Convert ProjectionBase to an interface

* Do not leak GL object in globe projection error measurement, add a destroy method to projection

* Fix chrome performance warning, refactor error measurement

Warning fixed by changing ring buffer size to 1, making ring buffer pointless, so I removed it.

* Fix granularity capitalization

* Fix capitalization

* Fix typo

* Fix stencil mask triangle index order (this was causing failing render tests)

* Cleanup vertex shader projection interface

* Move projection creation function into its own file

* Remove getProjectionName

* Added comment for deduplicateWrapped

* Remove unused vertex-buffer-related code from image source

* Add globe raster layer render test

* More render tests - test transition to mercator

* Remove pointless test, add test descriptions

* Render test for rendering poles on globe

* SubdivisionGranularitySetting constructor takes an object

* Remove "defines" parameter from useProgram

* Refactor useProgram and Program constructor

* Properly format translatePosMatrix comment

* Refactor globe-specific code outside projection classes, remove stencil-specific granularity settings

* Refactor granularity settings to be more readable

* Minor refactor of ProjectionErrorMeasurement

* Refactor draw_raster.ts

* Move globe utility functions to utils.ts, use easeCubicInOut instead of smoothStep

* Simplify imports in globe.ts

* globe.ts refactor

* Move ProjectionErrorMeasurement to a separate file

* Refactor ProjectionErrorMeasurement

Change parseRGBA8float to a private static function, use isWebGL2 function instead of instanceof

* Refactor draw_raster.ts

* Refactor globe projection error measurement to not use Painter

* Painter.clearStencil creates custom ProjectionData instead of calling getProjectionData(null, null)

* Remove "deduplicateWrapped" functionality from source_cache.ts

* Globe projection no longer requires a map instance

* Painter doesn't pass `this` to `updateGPUdependent`

* isRenderingDirty is now a function

* Rename ProjectionBase to Projection

* Replace globeView property with setGlobeViewAllowed

* Add mercator and globe projection unit tests

* Remove tests that test for exact clipping planes

* Update build test with new bundle size

* isRenderingDirty is now a function

* Fill, fill-extrusion, line layers, subdivision: Import changes from kubapelc/globe-vector branch

* Fix unit tests

* Subdivision: ensure consistent triangle winding order, fix unit tests

* Fix terrain

* Fix fill extrusion not working with terrain

* Fix typos

* Fix line gradient bug

* Subdivision: fix line ring handling

* Subdivision: fix unit test expecting an invalid line segment

* Fix fill-extrusion ring handling

* Fill-extrusion refactor and fix failing test

* Update terrain fill extrusion test expected image

* Render tests for fill, line and fill-extrusion for globe

* Move fillArrays function into a separate file

* Add vector globe example

* Remove changes for line and fill-extrusion layers to make the PR smaller

* Add unit tests for fillArrays()

* fillArrays unit test has better segment size limits

* Update build test build size

* Fix html example description

* Fix missing docs for granularity settings

* Rename globe fill render test tile source layer to "vector_tiles"

* Fix classifyRings comment format

* Move subdivisionGranularitySettingsNoSubdivision constant to a static readonly field, shorten the name

* Use `import type` for SubdivisionGranularitySetting where possible

* Fix typo

* Revert fill_attributes back to default exports

* Improve comment for scanline subdivision

* Subdivision: break up scanline subdivision function into more functions

* Move SubdivisionGranularitySetting into its own file

* Unit tests: use mock of MercatorProjection instead of the full class

* Add SegmentVector unit tests

* Subdivision: unit tests for poles, ring triangulation, fix bug in ring triangulation

* Subdivision: more pole unit tests

* Subdivision: fix wireframe generation, add unit test for wireframe

* Rename subdivisionGranularitySettings.ts to subdivision_granularity_settings.ts

* Move granularity settings registration to subdivision

* Update build size

* Rename `fillArrays` to `fillLargeMeshArrays`

* Move virtual buffers to a test util file

* Better warning for segments.ts vertex overflow

* Better comment for projection subdivision granularity

* Clarify mesh comparison in fill_large_mesh_arrays.test.ts

* Move mesh creating functions into a separate file, add tests for mesh comparison and grid creation

* Refactor and add better doc comment for `fillLargeMeshArrays`

* Refactor fill_large_mesh_arrays by removing duplicated code

* Move debug functions to mesh_utils.ts

* Unit tests: use StructArrays instead of VirtualVertexBuffer, etc.

* Subdivision: refactor

* Subdivision: rename subdivideFill to subdividePolygon, remove wireframe function

* Subdivision: throw when a vertex is outside int16 range

* Subdivision: refactor generatePoleQuad into a proper function

* Subdivision: add subdivision benchmark

* Subdivision: split scanline subdivision into smaller functions

* Remove wireframe generation function

* Subdivision: better doc comments for scanline subdivision

* Fix 'as any' in segment.ts

* Reuse condition in fill_large_arrays

* Deduplicate code in fill_large_arrays

* Subdivision: remove redundant function in tests

* Subdivision: improve scanline subdivision comments

* Subdivision: benchmark is not async

* Rename SegmentVector's invalidateLast to forceNewSegmentOnTextPrepare

* More tests for segment.ts

* Fix typo in forceNewSegmentOnNextPrepare

* Subdivision: more tests for fillLargeMeshArrays

* Subdivision: better comment in fillLargeMeshArrays
* Fix merge

* Import line layer changes from kubapelc/globe-vector

* Lines: shorten line_bucket.test.ts subdivision settings

* Lines: minor refactor

* Lines: update build size

* Lines: minor refactor
@HarelM HarelM marked this pull request as ready for review April 10, 2024 17:36
@HarelM
Copy link
Collaborator Author

HarelM commented Apr 10, 2024

This is still in draft, but I wanted coverage support, which doesn't work well for some reason...

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 87.52467% with 632 lines in your changes missing coverage. Please review.

Project coverage is 88.05%. Comparing base (87486a5) to head (9e105c3).
Report is 22 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/geo/projection/globe_transform.ts 79.65% 142 Missing and 23 partials ⚠️
src/geo/projection/globe_camera_helper.ts 64.17% 110 Missing and 10 partials ⚠️
src/data/bucket/fill_extrusion_bucket.ts 9.17% 97 Missing and 2 partials ⚠️
src/render/subdivision.ts 91.39% 35 Missing and 16 partials ⚠️
src/geo/projection/mercator_transform.ts 94.01% 26 Missing and 9 partials ⚠️
src/geo/projection/globe_utils.ts 81.51% 20 Missing and 2 partials ⚠️
...o/projection/globe_projection_error_measurement.ts 88.69% 15 Missing and 4 partials ⚠️
src/data/bucket/circle_bucket.ts 70.58% 13 Missing and 2 partials ⚠️
src/render/painter.ts 86.84% 12 Missing and 3 partials ⚠️
src/geo/transform_helper.ts 96.02% 6 Missing and 7 partials ⚠️
... and 23 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3963      +/-   ##
==========================================
+ Coverage   87.96%   88.05%   +0.08%     
==========================================
  Files         247      265      +18     
  Lines       33656    37577    +3921     
  Branches     2158     2333     +175     
==========================================
+ Hits        29605    33087    +3482     
- Misses       3094     3462     +368     
- Partials      957     1028      +71     

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

@HarelM
Copy link
Collaborator Author

HarelM commented Apr 10, 2024

@kubapelc I've rigged codecov to work on this PR, feel free to see which classes are covered well and which need more work.
I'll keep this PR open so that we can get up-to-date coverage information on every PR merge.

HarelM and others added 4 commits April 11, 2024 08:43
* Import changes for fill-extrusion from main vector globe branch

* Fill extrusion: refactor

* Fill extrusion: indent shader ifdefs

* Fill extrusion: add example

* Fill extrusion: update build size

* Move globe specific projection methods to projection interface

* Fix failing unit test

* Use vec3.clone() instead of manually copying vector components
* Import background layer changes from main vector globe branch

* Import hillshade layer changes from main vector globe branch

* Subdivision: explicit types

* Fix single-pixel seams in the oceans

* Add render test for background pattern on globe

* Refactor drawBackground

* Refactor drawHillshade

* Update build size

* Update globe background-pattern render test with results from CI

* Hillshade: refactor prepareHillshade

* Add a render test for fill layer seams fix
@HarelM HarelM marked this pull request as draft April 17, 2024 06:54
HarelM and others added 3 commits April 18, 2024 09:01
* Import changes for circle and heatmap layers from the main vector globe branch

* Minor refactors

* Update build size

* Use "/ 8.0" in shader instead of "* 0.125"

* Update shader comments

* Use a thin type instead of full Transform in projection

* Only import types in projection.ts

* getPixelScale and getCircleRadiusCorrection only need map center as argument

* Only import types where possible in projection classes

* Smaller refactors

* Fix failing unit test

* Add heatmap render test

* More explicit types in projection interface

* Globe plane equation is a vec4

* Fix wrong args in projection functions
@HarelM
Copy link
Collaborator Author

HarelM commented May 15, 2024

@kubapelc, I've merged main into this branch.
I've left some "HM TODO" in places where I replaced some logic that was needed.
Please check this thoroughly.

@HarelM
Copy link
Collaborator Author

HarelM commented May 15, 2024

Bahhh.... I messed this one up.
Let me know if you need me to do anything to resolve this, or if you are taking care of it...

HarelM and others added 3 commits May 15, 2024 16:16
* Import changes from main vector globe branch

* Fix import

* Remove unused code

* Remove unused imports

* Update build size test

* Remove unused function

* Add render test results for Debian

* Add another Debian render test variant

* Add more render test variants

* Hide collision boxes on the backfacing side of the globe

* Fix pitch-aligned texts getting hidden when their anchor is beyond horizon

* Update build size

* Fix merge

* Better comment in draw_collision_debug

* Update build size

The 10 kb size increase seems to come from the main branch

* Minor refactor

* Use explicit types, even for unused parameters

* Refactor screenspace path projection

* Refactor imports for projection.ts and collision_index.ts

* Fix import in collision_index.ts
@HarelM
Copy link
Collaborator Author

HarelM commented May 20, 2024

@kubapelc what still missing for this branch besides the atmosphere and the style spec definition for globe?

@HarelM
Copy link
Collaborator Author

HarelM commented May 20, 2024

Note that there were examples that were added, which will appear in the docs site.
For those to appear as expected there's a need to generate images for them using the generate image script.
The following are the warning from the build generation code:

WARNING -  Doc file 'examples/index.md' contains a link '../assets/examples/globe-fill-extrusion.png', but the target 'assets/examples/globe-fill-extrusion.png' is not found among documentation files.
WARNING -  Doc file 'examples/index.md' contains a link '../assets/examples/globe-vectortiles.png', but the target 'assets/examples/globe-vectortiles.png' is not found among documentation files.
WARNING -  Doc file 'examples/index.md' contains a link '../assets/examples/globe.png', but the target 'assets/examples/globe.png' is not found among documentation files.

See here:
https://github.com/maplibre/maplibre-gl-js/blob/main/docs/README.md#writing-examples

@kubapelc
Copy link
Collaborator

what still missing for this branch besides the atmosphere and the style spec definition for globe?

Everything rendering related on my part is basically done and merged, what remains is adapting the rest of MapLibre to work with globe projection. Most notably controls need to be adapted for globe, then animations such as flyTo, placing custom HTML at a location, custom layer API for globe and for placing 3D objects onto the globe (as mentioned in the globe discussion), and in general everything that projects screen pixels to coordinates or vice versa. I'm working on it, it's a long list, but I think (hope) it should be much less complex than the rendering part...

Also #3887 should be implemented for the globe to be usable.

@HarelM
Copy link
Collaborator Author

HarelM commented Sep 23, 2024

When above pr lands then we'll probably have to expose the second projection setting for the high zoom.

Maybe we can name it globe-mercator to indicate both are used, IDK.
We can keep it experimental for now I believe and see how to better define this in order to avoid backward compatibility issues...?

@HarelM
Copy link
Collaborator Author

HarelM commented Sep 23, 2024

@Samarth1696 @cutephoton I've merged main branch into here, Please let me know if the fixes you have made are still OK in this branch, my main concern in the heatmap fix to be honest, I hope the tests added in the PR are covering this feature well to indicate if my merge was correct or not.

@kubapelc
Copy link
Collaborator

I think globe-mercator is a good name. As for setting globe outside the style object, I think it would be nice, since it works mostly seamlessly with mercator maps. I'm not sure how exactly the API shoul look...

High zoom globe uses mercator in particular because globe and mercator projections converge at high zooms. In this context "globe projection" is a classical perspective projection camera looking at a perfect sphere, and moving closer to the surface with increasing zoom. I'm not sure projections other than mercator have this property or that something would be gained by using them instead of mercator. But I can definitely see a use case for accepting tiles that are not web mercator, but that is outside the scope of globe implementation.

@birkskyum
Copy link
Member

birkskyum commented Sep 23, 2024

High zoom globe uses mercator in particular because globe and mercator projections converge at high zooms. In this context "globe projection" is a classical perspective projection camera looking at a perfect sphere, and moving closer to the surface with increasing zoom. I'm not sure projections other than mercator have this property or that something would be gained by using them instead of mercator. But I can definitely see a use case for accepting tiles that are not web mercator, but that is outside the scope of globe implementation.

@kubapelc the reason #168 is so highly requested is that a lot of governmental GIS services opt for non-mercator projections that have lower distortion/higher accuracy in the local region they cover. Take an example like this App (most countries have something similar) by the Environmental Ministry of Denmark, which is built on OpenLayers. If the cookie bar is removed, in the lower right it says 'ETRS89 EAST / NORTH', which is the this projection, which is more suitable for Europe than Mercator. Furthermore, most counties have a projection tailor to just the single country, and even the extreme is the construction industry who need high-accuracy for floor plans of single buildings etc., so they'll i.e. use 3 low-distortion projection slices across a small country like Denmark.

@Samarth1696
Copy link
Contributor

@Samarth1696 @cutephoton I've merged main branch into here, Please let me know if the fixes you have made are still OK in this branch, my main concern in the heatmap fix to be honest, I hope the tests added in the PR are covering this feature well to indicate if my merge was correct or not.

The things you changed looks correct to me!

But I have some concerns regarding terrain with the globe. I don't know maybe my configuration might be incorrect. Feel free to correct me if I am wrong. I am getting this on poles when I have set Terrain off:
{183CA944-EC13-4146-88B0-85A6BE6BE9E7}
When I have set the Terrain on:
{8979C2FE-58FD-4936-BBA5-4E930284A7D0}
Maybe there is some issue in the projection with raster layers?

I used this configuration to test:

<!DOCTYPE html>
<html lang="en">
<head>
    <title>Display a globe with a vector map</title>
    <meta property="og:description" content="Display a globe with a vector map." />
    <meta charset='utf-8'>
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <link rel='stylesheet' href='../../dist/maplibre-gl.css' />
    <script src='../../dist/maplibre-gl-dev.js'></script>
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>
<body>
<div id="map"></div>
<script>
    const map = new maplibregl.Map({
        container: 'map',
        zoom: 14.66,
        center: [-112.0392, 33.54002],
        pitch: 66,
        hash: true,
        style: {
            version: 8,
            sources: {
                osm: {
                    type: 'raster',
                    tiles: ['https://a.tile.openstreetmap.org/{z}/{x}/{y}.png'],
                    tileSize: 256,
                    attribution: '&copy; OpenStreetMap Contributors',
                    maxzoom: 19
                },
                // Use a different source for terrain and hillshade layers, to improve render quality
                terrainSource: {
                    type: 'raster-dem',
                    url: 'https://api.maptiler.com/tiles/terrain-rgb/tiles.json?key=API_KEY',
                    tileSize: 256
                },
                hillshadeSource: {
                    type: 'raster-dem',
                    url: 'https://api.maptiler.com/tiles/terrain-rgb/tiles.json?key=API_KEY',
                    tileSize: 256
                }
            },
            layers: [
                {
                    id: 'osm',
                    type: 'raster',
                    source: 'osm'
                },
                {
                    id: 'hills',
                    type: 'hillshade',
                    source: 'hillshadeSource',
                    layout: {visibility: 'visible'},
                    paint: {'hillshade-shadow-color': '#473B24'}
                }
            ],
            terrain: {
                source: 'terrainSource',
                exaggeration: 1
            },
            sky: {}
        },
        maxZoom: 18,
        maxPitch: 85
    });

    map.on('style.load', () => {
        map.setProjection({
            type: 'globe', // Set projection to globe
        });
    });

    map.addControl(
        new maplibregl.NavigationControl({
            visualizePitch: true,
            showZoom: true,
            showCompass: true
        })
    );

map.addControl(
        new maplibregl.TerrainControl({
            source: 'terrainSource',
            options: {
        exaggeration: 2,
        elevationOffset: 0
      }
     })
    );
</script>
</body>
</html>

Copy link
Contributor

@cutephoton cutephoton left a comment

Choose a reason for hiding this comment

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

@HarelM it looks like it should work. I verified the shaders and confirm they look correct. I did the eyeball check using multiple geometries example. It was ok. The render tests are taking an eternity. I am seeing a couple failing tests sporadically. I'll post an update when that eventually finishes.

Copy link
Contributor

@cutephoton cutephoton left a comment

Choose a reason for hiding this comment

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

Tested with --open-browser. No issues with circles. Failed tests do not seem related to the changes here, fail on main and are only ever so mildly outside of expectations on the few tests that failed.

 1/7: failed tests/text-local-ideographs/cjk 0.0021619791666666666 
 2/7: failed tests/raster-masking/overlapping-zoom 0.003936767578125 
 3/7: failed tests/projection/globe/raster-warped 0.054424285888671875 
 4/7: failed tests/projection/globe/raster-pole 0.000293731689453125 
 5/7: failed tests/debug/tile-raster 0.0036478042602539062 
 6/7: failed tests/debug/tile 0.00693511962890625 
 7/7: failed tests/debug/raster 0.0161285400390625 

@Pessimistress
Copy link
Contributor

Should CustomRenderMethodInput include globeness for third-party integration?

https://github.com/maplibre/maplibre-gl-js/blob/d0981a07962dfcf8dee077621bd12ca0fec0623f/src/render/draw_custom.ts#L17C28-L17C51

Thinking of future functionalities, this transition factor may be needed by other projections as well. Maybe "mercatorness" would be a more generic name?

@HarelM
Copy link
Collaborator Author

HarelM commented Sep 29, 2024

That's a good question, I don't have a good answer, but I do want to merge this.
So I'll move all the relevant stuff that are still not solved to the following issue and release a pre-release of version 5:

@HarelM
Copy link
Collaborator Author

HarelM commented Sep 30, 2024

Here we go...

@HarelM HarelM merged commit a283af1 into main Sep 30, 2024
15 checks passed
@HarelM HarelM deleted the globe branch September 30, 2024 07:16
@kubapelc
Copy link
Collaborator

Hi @Pessimistress the globeness parameter is exposed in CustomRenderMethodInput, it is in defaultProjectionData.projectionTransition. I'll think about how to document this better.

I think that the globeness name is only used in globe-related classes, all generic code (just shaders, really) uses projectionTransition.

@kubapelc
Copy link
Collaborator

I'm working on one more globe-related PR. It has some optimizations, mostly around symbol placement.

@HarelM
Copy link
Collaborator Author

HarelM commented Sep 30, 2024

Thanks for the update, feel free to take a look at the list I wrote here

At the beginning of the issue I wrote a check list of things that should be added/solved as part of version 5 release. feel free to continue the discussion there.

@laem
Copy link

laem commented Sep 30, 2024

It's online on https://cartes.app :) Thanks for the great work.

@russss
Copy link

russss commented Sep 30, 2024

I also just put it live on OpenInfraMap. It all looks good except for #4781. Thanks again for all the hard work!

@eikes
Copy link

eikes commented Oct 10, 2024

I have been playing with this and it is such a delight! Thank you very much! It would be nice to have an example where this is used.

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.

Globe view - Earth is not flat