Globe final PR

[HarelM]

Launch Checklist

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

• [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.
• [ ] Link to related issues.
• [ ] Include before/after visuals or gifs if this PR includes visual changes.
• [x] 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.

[HarelM]

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

[codecov-commenter]

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.

:white_check_mark: 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 :warning:
src/geo/projection/globe_camera_helper.ts	64.17%	110 Missing and 10 partials :warning:
src/data/bucket/fill_extrusion_bucket.ts	9.17%	97 Missing and 2 partials :warning:
src/render/subdivision.ts	91.39%	35 Missing and 16 partials :warning:
src/geo/projection/mercator_transform.ts	94.01%	26 Missing and 9 partials :warning:
src/geo/projection/globe_utils.ts	81.51%	20 Missing and 2 partials :warning:
...o/projection/globe_projection_error_measurement.ts	88.69%	15 Missing and 4 partials :warning:
src/data/bucket/circle_bucket.ts	70.58%	13 Missing and 2 partials :warning:
src/render/painter.ts	86.84%	12 Missing and 3 partials :warning:
src/geo/transform_helper.ts	96.02%	6 Missing and 7 partials :warning:
... 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     

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

[HarelM]

@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]

@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]

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]

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

[HarelM]

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]

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 https://github.com/maplibre/maplibre-gl-js/issues/3887 should be implemented for the globe to be usable.

[kubapelc]

I'll fix the globe examples.

[HarelM]

Thanks for the info! If you happen to resolve the following enhancement it will be great:

• #1596

[HarelM]

Please also take a look at the coverage report. I think some unit tests will be helpful here to cover more of the code scenarios as render test are good but are not suitable for edge cases. I would recommend aiming to 80-90% for each file so that the overall coverage would stay around 80-90%. Or less than 100 lines not covered as part of this PR.

[KookaburraShell]

It seems that the coordinates for placed markers seem to not work correctly. When you rotate the globe they move out of screen instead of rotating with the globe.

Seems like that mapbox had the same issue when implementing their globe, perhaps this might help mapbox/mapbox-gl-js/pull/11556

Uploading demo.mp4…

[kubapelc]

Hello, in this branch only the rendering of layers is adapted for globe, other things still behave as if they were mercator. I'm working on fixing that right now, and proper marker behaviour is definitely on my TODO-list, as well as better controls for globe :)

[HarelM]

@kubapelc there's a small difference in a render test after merging a fix from main branch related to elevation of polygon. See this PR:

• #3841

It's a very small fix to move the centroid calculation into the for loop instead of outside. The diff is very small, but noticeable in the render test. My gut feeling is that we should update the expected render test image, but let me know what you think.

[kubapelc]

Hello, this line:

const oldVertexCount = this.layoutVertexArray.length;

at fill_extrusion_bucket.ts line 171 should be moved into the for loop, so that the resulting code looks roughly like this:

addFeature(feature: BucketFeature, geometry: Array<Array<Point>>, index: number, canonical: CanonicalTileID, imagePositions: {[_: string]: ImagePosition}, subdivisionGranularity: SubdivisionGranularitySetting) {
        for (const polygon of classifyRings(geometry, EARCUT_MAX_RINGS)) {
            // Compute polygon centroid to calculate elevation in GPU
            const oldVertexCount = this.layoutVertexArray.length; // <---- moved here
            const centroid: CentroidAccumulator = {x: 0, y: 0, sampleCount: 0};

The for-loop that later iterates over addedVertices needs to fill the centroid center for every vertex added in processPolygon, and it uses oldVertexCount to compute how many new vertices were added. If oldVertexCount was initialized outside the classifyRings for-loop, it would instead track how many vertices were added for each ring/polygon so far in total.

As for the render test, this oldVertexCount change does not affect it. I think the test is different because the globe code takes into account that polygons sometimes do (and sometimes don't) have their first/last vertex duplicated. If it is duplicated, globe code ignores it in centroid computation. I think the main branch does not do this, and incorrectly counts the first/last vertex twice, thus it computes a slightly different centroid.

So updating the render test expected image is correct.

[HarelM]

Thanks for the tip, I have moved the above line, I'll see if the render test still fails and update the image if needed.

[AgentSmith0]

Hi, how can I define the projection in the options? And is there a way to change the projection after initialization? (setProjection and the projection attribute don't do anything)

[kubapelc]

Hi, how can I define the projection in the options? And is there a way to change the projection after initialization? (setProjection and the projection attribute don't do anything)

Hi, I think something like this should work:

map.on("load", () => {
    map.style.setProjection({type: "globe"});
});

But this will only set the projection once the map is loaded, but globe needs tiles to be reloaded if that happens and afaik this globe branch doesn't yet have a mechanism to trigger that, so you will get artifacts. This is already fixed in my dev branch.

Edit: a better way exists if you are loading the map from some URL, say maptiler:

fetch('https://api.maptiler.com/maps/streets/style.json?key=your_api_key').then(response => response.json()).then(fetchedStyle => {
	fetchedStyle["projection"] = {"type": "globe"};
	map = new maplibregl.Map({
		container: 'map',
		style: fetchedStyle,
	});
});

This should not produce any artifacts, since the map would start out under globe projection already.

[HarelM]

Seems like I broke some of the terrain tests with the latest merge from main... :-/ I'll look at it later on...

[HarelM]

@kubapelc don't forget to resolve conflicts here first as I see there are conflict with transform class and recent changes/fixes.

[HarelM]

@kubapelc I've updated this branch with the fixes from main. The main conflict was around the collision index projectAndGetPerspectiveRatio which is now unoptimized again since there was a lot of code changes there related to globe and mercator projection, not sure I know how to solve this, as copying the code now is a lot more complicated...

[willemarcel]

As I commented in https://github.com/maplibre/maplibre-gl-js/discussions/1598#discussioncomment-10538001, the globe view doesn't render the geojson lines correctly when close to the poles. This is a simplified version of the data that can be used to reproduce this issue: https://gist.github.com/willemarcel/4995190eaf24e408b188a4d6fefca560

[HarelM]

@kubapelc what's still missing in order to merge this? I thought about merging this and creating a version 5 pre-release, do you think it's possible?

[kubapelc]

The only thing left that I know about is transferring the symbol optimizations from main to globe branch. Sadly I don't have much time at the moment, but I should be able to do a PR around the end of September.

[HarelM]

I see, that's very good news! I'll take this branch for a spin and see that it works as expected. If everything is in order, I'll merge it. Thanks again for all the hard work on this! Truly, truly appreciated!

[HarelM]

@Pessimistress, in case you want to check things out before this is merged (in continue to the conversation we had yesterday). I'm planning on merging this and create the first pre-release of version 5 of maplibre.

[neodescis]

@Pessimistress, in case you want to check things out before this is merged (in continue to the conversation we had yesterday). I'm planning on merging this and create the first pre-release of version 5 of maplibre.

I'd be happy to test out the prerelease version! However, I'm thinking we should do a final 4.x release with the latest fixes, just before merging this.

[HarelM]

Sure, currently there's only a little fix left that was not included in 4.7. I'll go over other PRs, maybe we could include some of those before this merge...

[laem]

Happy to test the prerelease version too on https://cartes.app as soon as it lands :)

Edit : works well, see demo gif on attached PR, but I couldn't figure out how to make the globe and the sky work together.

[birkskyum]

I tried out the cartes app, and it's really cool!

After having this api in the hands, and scanning through the PR changes in cartes, I got a bit concerned we might have made a design mistake here by nesting the projection setting inside of the style:

• https://github.com/maplibre/maplibre-style-spec/pull/687
• https://maplibre.org/maplibre-style-spec/projection/

This would mean that in order to use the globe, we'd need modify or duplicate all styles upstream, such as https://demotiles.maplibre.org/style.json, or download them, manually modify the object, and set that js object as the style.

What we could do instead, is to move the projection setting out of the style so it's next to the other viewport settings (center, pitch etc.). That would allow opting into the globe-mode for all the existing styles people use today.

It would also allow for the getter/setter mentioned in the Design Proposal in a consistent way (like getPitch/setPitch), where the current design technically makes it a part of setStyle():

APIs for setProjection and getProjection should be added to the map object.

What do you think of this idea?