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

Add contour source #623

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Add contour source #623

wants to merge 7 commits into from

Conversation

msbarry
Copy link

@msbarry msbarry commented Apr 23, 2024

Add a new source type that generates contour isolines from raster-dem sources.

Fixes #583 - see that issue for the full design and discussion.

Launch 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.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.
  • Major vs. index terminology?
  • Use expression for interval and major/index that only support ["zoom"] as an argument
  • should updating a raster-dem source used by a contour source cause remove/add on both sources?

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.54%. Comparing base (c755d44) to head (101e4fc).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #623      +/-   ##
==========================================
+ Coverage   92.23%   92.54%   +0.31%     
==========================================
  Files         100      105       +5     
  Lines        4196     4666     +470     
  Branches     1203     1316     +113     
==========================================
+ Hits         3870     4318     +448     
- Misses        326      348      +22     

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

@msbarry
Copy link
Author

msbarry commented Apr 23, 2024

I'll leave this open as a draft as I implement the gl-js side of this.

@HarelM
Copy link
Collaborator

HarelM commented Apr 23, 2024

This looks great overall, the only question left is the expression for the intervals and multiplier.
Thanks!!!

@@ -0,0 +1,136 @@
# Expressions

The value for any `layout property`, `paint property`, or `filter may be specified as an _expression_. An expression defines a formula for computing the value of the property using the _operators_ described below. The set of expression operators provided by MapLibre includes:
Copy link
Contributor

Choose a reason for hiding this comment

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

please use `filter` rather than `filter

@HarelM
Copy link
Collaborator

HarelM commented May 30, 2024

@msbarry any updates on this? Is there something I can help with?

@msbarry
Copy link
Author

msbarry commented May 31, 2024

I was planning to keep this open as I implement the maplibre-gl-js side of it in case I have to make any changes here. I got a bit sidetracked trying to get a planetiler release out with parquet/overture support but should have that wrapped up soon so I can get back to this.

My in-progress changes to maplibre-gl-js are here: https://github.com/msbarry/maplibre-gl-js/tree/contour-source - I have it working but it's pretty inefficient (overfetching a lot of tiles) and doesn't handle adding border pixels yet. Need to do some more thinking about how to handle those... It might make sense to refactor source_cache first to decouple staging the tile pyramid for a source consumer from fetching/caching tiles if you had any ideas there? I think if terrain and hillshading could read from the same source, then it should be ready for contour tiles to read from it as well.

@HarelM
Copy link
Collaborator

HarelM commented May 31, 2024

No rush, just wanted to make sure you haven't forgot about it 🙂
I don't have a good perspective about source cache ATM in order to retractor it, so you'll probably beat me to it...

@HarelM
Copy link
Collaborator

HarelM commented Oct 22, 2024

@msbarry where are we with this?
We are in the middle of 5.x version of maplibre, so this might be a good opportunity to break things if they are needed as part of this feature.

@msbarry
Copy link
Author

msbarry commented Oct 26, 2024

I'm still hoping to do this eventually, but it's gotten bumped further down my todo list in the short term 😞 I had been thinking of this as a purely additive thing. The only change to existing functionality would be if I can fix source cache and remove the need to define the same DEM source twice to use across terrain and hillshading, but that's just a convention change, not breaking. Was there anything you were thinking?

@HarelM
Copy link
Collaborator

HarelM commented Oct 31, 2024

I'm not sure what we need to break if at all in order to implement this feature. I recall stuff that might be related to source cache, although source cache is not a public API, but still.
If the feature doesn't require any changes, we can introduce it later on, in a minor release.
But since you looked at the code to implement it, you might have more insights as to what needs to change in order to implement this smoothly.
In any case, I hope the following PR would allow for most of what you need:
maplibre/maplibre-gl-js#4779

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.

Design Proposal: Contour Line Source from Raster DEM Tiles
4 participants