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 + 3d terrain + satellite example #4926

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Oct 29, 2024

Combining Hybrid Satellite + Globe + 3d Terrain, to see how photorealistic maplibre gl js can be.

Demo:
https://codepen.io/birkskyum-1471370946/pen/xxvWmoG

Affected by

Screenshot 2024-10-29 at 14 50 44
  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.55%. Comparing base (ea3961c) to head (fbd45e9).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4926      +/-   ##
==========================================
- Coverage   90.69%   90.55%   -0.15%     
==========================================
  Files         265      265              
  Lines       38118    38118              
  Branches     3141     3169      +28     
==========================================
- Hits        34573    34518      -55     
- Misses       2595     2645      +50     
- Partials      950      955       +5     

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

@birkskyum birkskyum marked this pull request as draft October 29, 2024 13:40
@birkskyum birkskyum marked this pull request as ready for review October 29, 2024 13:40
@birkskyum birkskyum changed the title Globe + 3d terrain example Globe + 3d terrain + satellite example Oct 29, 2024
<div id="map"></div>
<script type="module">

async function downloadStyleObject(url) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we simplify this by just writing a short inline style without using an external style?

Copy link
Member Author

Choose a reason for hiding this comment

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

I managed to rewrite this with transformStyle

Copy link
Member Author

@birkskyum birkskyum Oct 29, 2024

Choose a reason for hiding this comment

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

The hybrid satellite layer from maptiler has many layers, I'm not sure how I can achieve the same with a short inline style. This example is intentionally a bit advanced, to pressure test the capabilities of maplibre

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend using a 2-5 critical layers for the example, instead of complicating it with a style that you override...

Copy link
Member Author

@birkskyum birkskyum Oct 29, 2024

Choose a reason for hiding this comment

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

Okay, I've now removed ~ 10 layers and inlined just the 4 main onces from maptiler that still quite well demonstrate what it's supposed to:

  • city label
  • road line
  • road label
  • sat raster tiles

The line count grew from 89 to 249 lines though, but there's no style transformation logic

Copy link
Member Author

@birkskyum birkskyum Oct 29, 2024

Choose a reason for hiding this comment

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

The layers are still somewhat complicated - the idea of the example is to focus on what you would like to highlight and not how to write styles.

I fully agree with this, which is why I've reverted the inline again. Having most of style in the maptiler url better show what I'm trying to highlight, which is the full extend of maplibre globe+terrain+hillshade+hybrid rendering capabilities, instead of focusing on how to write inline styles simpler, which cuts into the visual output I'm trying to highlight. This example, due to it's complexity, already surfaced 2 bugs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a style to the demo tiles repo with what's needed for this example?

Copy link
Member Author

@birkskyum birkskyum Oct 29, 2024

Choose a reason for hiding this comment

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

Yes, the full style, adding the globe + demotiles terrain to the stadia hybrid sat style is here:

Copy link
Member Author

@birkskyum birkskyum Oct 29, 2024

Choose a reason for hiding this comment

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

I've reached out about the licensing, to figure if we're allowed to take that approach - it does have attribution, but it would remove the tile service url from the example. Imo, these 30L of style modification isn't that troublesome, if the concern is that it's making the example hard to read. With a total of 89 lines, it's still one of the shorter examples. If we're intending to use this style in more places, which might be relevant for maketing (global raster-dem would be better though), then I see how it makes sense to reduce the duplicate code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's wait for a response, I think using a style file is a lot cleaner for what we would like to showcase here.

@kubapelc
Copy link
Collaborator

We have never actually implemented globe support for terrain, so if this works, it works by accident.

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.

4 participants