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

Use raster dem encoding #3087

Merged
merged 15 commits into from Oct 5, 2023
Merged

Conversation

ibesora
Copy link
Contributor

@ibesora ibesora commented Sep 16, 2023

This is uses maplibre-style-spec#337 to allow rendering raster-dem sources with an arbitrary encoding.

As an example, the first image is a QGIS reference render of a raster-dem with a different encoding, the second one is how it is rendered currently in Maplibre and the third one is how it's rendered using this PR.

Screenshot 2023-09-16 at 18 22 16 Screenshot 2023-09-16 at 17 17 45 Screenshot 2023-09-16 at 17 17 52

Tests are obviously failing because this is pointing to a maplibre-style-spec version that doesn't contain the needed changes

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.

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (dd3bea1) 75.09% compared to head (f44ebef) 75.11%.
Report is 32 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3087      +/-   ##
==========================================
+ Coverage   75.09%   75.11%   +0.02%     
==========================================
  Files         240      240              
  Lines       19150    19170      +20     
  Branches     4323     4325       +2     
==========================================
+ Hits        14380    14400      +20     
  Misses       4770     4770              
Files Coverage Δ
src/data/dem_data.ts 100.00% <100.00%> (ø)
src/source/raster_dem_tile_source.ts 52.50% <100.00%> (+2.50%) ⬆️
src/source/raster_dem_tile_worker_source.ts 64.28% <100.00%> (ø)

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

src/source/worker_source.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Member

HarelM commented Sep 16, 2023

This looks great!! THANKS!
I've added a few minor comments.

@ibesora ibesora force-pushed the ib/external-use-raster-dem-encoding branch from 5b9d787 to 5a6036d Compare October 2, 2023 16:15
@HarelM
Copy link
Member

HarelM commented Oct 3, 2023

Can you please add a changelog entry?
Otherwise this looks good.

@HarelM
Copy link
Member

HarelM commented Oct 3, 2023

Seems like there are some failing tests, so it might be that something is passed correctly...

@ibesora
Copy link
Contributor Author

ibesora commented Oct 3, 2023

Seems like there are some failing tests, so it might be that something is passed correctly...

Yup, looking at those now. Most of them are timing out so I'm wondering what's happening but I'll follow up with another commit once those are fixed

src/data/dem_data.ts Outdated Show resolved Hide resolved
@ibesora
Copy link
Contributor Author

ibesora commented Oct 4, 2023

@HarelM I need your take on the last fixes and the error that is still showing here

  1. I was getting a bunch of errors on all the hillshade tests. I have been looking at what was happening and it was a rounding error. Using the previous behavior we'd get a value like 1243 while with the new approach we get 1243.000000006 and that was causing the differences. I fixed this by adding a rounding step to 5 decimal places here but that's totally made up. Alternatively we could just update the test images.
  2. The style in tests/terrain/field-of-view was wrong. It contained a "format" key that's not valid according to the RasterDEM spec. This was not a problem before because RasterDEM was validated as an Object in maplibre-style-spec and that allowed for random keys. The update to the last version of the style spec changed the validation from an Object to a proper validation so it fails when there are invalid properties. This was fixed here
  3. And lastly, I had to increase the min size to make the build test pass
  4. On the error that's still showing, do you think we can just update the test result image? It seems like it's only a precision problem again

@HarelM
Copy link
Member

HarelM commented Oct 4, 2023

If the change in the images is not significant, I would consider simply updating the images.
It's weird that using variables instead of hardcoded numbers creates these rounding errors, I'm not sure rounding is the right approach though, but if we can't figure out the root cause of these small changes I guess rounding is OK.

@ibesora
Copy link
Contributor Author

ibesora commented Oct 5, 2023

It's weird that using variables instead of hardcoded numbers creates these rounding errors, I'm not sure rounding is the right approach though, but if we can't figure out the root cause of these small changes I guess rounding is OK.

It's not that, we are essentially doing more floating point operations than before. The previous Mapbox encoding formula was (r*256*256 + g*256 + b) * 0.1 - 10000.0 while the new one is r*6553.6 + g*25.6 b*0.1 - 10000.0. The first one results in 832150.4while the second one results in 832150.4000000001.

Anyway, it seems that the errors I was seeing were not related to that so I just removed the rounding and the tests are still passing so I think this is ready to merge now!

@HarelM HarelM merged commit e5dd5a0 into maplibre:main Oct 5, 2023
14 checks passed
@HarelM
Copy link
Member

HarelM commented Oct 5, 2023

Damn, changelog entry is still missing, can you please open a new PR with the addition of this feature in the changelog?

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

3 participants