Skip to content

Validate that raster layers do not specify both 'resampling' and 'raster-resampling'#1532

Merged
HarelM merged 5 commits intomaplibre:mainfrom
larsmaxfield:feature/resampling-warning
Mar 8, 2026
Merged

Validate that raster layers do not specify both 'resampling' and 'raster-resampling'#1532
HarelM merged 5 commits intomaplibre:mainfrom
larsmaxfield:feature/resampling-warning

Conversation

@larsmaxfield
Copy link
Contributor

Changes:

  • If a raster layer specifies both 'resampling' and 'raster-resampling' paint properties, raise a ValidationError

In implementing 'resampling' in maplibre-gl-js, @HarelM and I concluded that the style spec should be responsible for catching when a raster layer specifies both 'resampling' and 'raster-resampling'.

Launch Checklist

  • Briefly describe the changes in this PR.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.79%. Comparing base (8d85feb) to head (ef2edbb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1532   +/-   ##
=======================================
  Coverage   93.78%   93.79%           
=======================================
  Files         112      112           
  Lines        4668     4670    +2     
  Branches     1571     1572    +1     
=======================================
+ Hits         4378     4380    +2     
  Misses        290      290           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@CommanderStorm CommanderStorm requested a review from HarelM March 8, 2026 11:03
@larsmaxfield
Copy link
Contributor Author

I did not update the resampling "sdk-support" because the implementation in GL JS has only been merged, not yet released. Does it make any sense to preemptively specify 5.20.0? Or instead wait until it's released and then merge this? (Or something else?)

@CommanderStorm
Copy link
Member

we can do that now, this way it is out of the way

@larsmaxfield
Copy link
Contributor Author

Done!

Are the .output-api-supported.json files updated in a special way? I only had layers.output.json change when I ran the layer test with the UPDATE=1 flag, and I don't know whether I should expect its output-api-supported counterpart to change as well.

@HarelM
Copy link
Collaborator

HarelM commented Mar 8, 2026

Looking at the code you added, this looks as expected. When you update the layers.input.json and run update=1 the output should change according to the changes you created in the input.
Was the process not working as expected?

@larsmaxfield
Copy link
Contributor Author

Ok that's good to hear. And it's my lack of familiarity with the codebase that's the issue 😄 I couldn't find what distinguishes output-api-supported from simply output so I was unsure if I missed something.

@HarelM
Copy link
Collaborator

HarelM commented Mar 8, 2026

Oh, I understand the question now. I'm not sure they are in use anymore.
I wonder when we stopped using them though...

@larsmaxfield
Copy link
Contributor Author

Ah ok 👍

@HarelM
Copy link
Collaborator

HarelM commented Mar 8, 2026

Created a PR to remove them:

To avoid future confusion.
I think they were used with mapbox related stuff (mapbox://), but they never got to be used in this repo when we took this part out of maplibre-gl-js.

@HarelM HarelM merged commit ba751f4 into maplibre:main Mar 8, 2026
8 checks passed
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