add raster-resampling raster paint property #6411

andrewharvey · 2018-03-28T05:36:30Z

Launch Checklist

briefly describe the changes in this PR
fixes Add style-spec option to specify interpolation mode for raster-style sources #4450 adding a raster-scaling raster paint property
Although the ticket described doing this for Image sources only, I think it should be generic to any raster layer, working across Image, Video, Canvas and Raster (noticeable where the layer maxzoom is greater than the source maxzoom) sources.
https://csantosbh.wordpress.com/2014/01/25/manual-texture-filtering-for-pixelated-games-in-webgl/ talks about ways to remove the aliasing created using a NEAREST filter but still getting the same pixelated affect.
write tests for all new functionality
- get the implementation working to add the fixtures for the render tests
document any changes to public APIs
post benchmark scores
manually test the debug page
- raster sources ✅
- image sources ✅

tested both raster and image sources with linear and nearest with:

map.setPaintProperty('image', 'raster-scaling', 'nearest')
map.setPaintProperty('image', 'raster-scaling', 'linear')

andrewharvey · 2018-03-28T05:37:46Z

src/source/image_source.js

@@ -198,7 +198,7 @@ class ImageSource extends Evented implements Source {

        if (!this.texture) {
            this.texture = new Texture(context, this.image, gl.RGBA);
-            this.texture.bind(gl.LINEAR, gl.CLAMP_TO_EDGE);
+            this.texture.bind(gl.NEAREST, gl.CLAMP_TO_EDGE);


this is just temporary to see 'nearest' working for Image sources. See comment in the PR.

andrewharvey · 2018-04-07T08:06:46Z

I'll pick this up again once #6444 is merged to see if I have any more luck.

andrewharvey · 2018-04-20T01:51:29Z

I'll pick this up again once #6444 is merged to see if I have any more luck.

#6444 seemed to fix the issue I was running into here. everything looks fine now, ready for review please.

andrewharvey · 2018-04-21T04:05:19Z

I presume since 0.45 beta was just released this won't make it into 0.45? If so I'll update this PR to target 0.46.

andrewharvey · 2018-05-01T23:43:41Z

I presume since 0.45 beta was just released this won't make it into 0.45? If so I'll update this PR to target 0.46.

I've updated the PR to target 0.46.0.

andrewharvey · 2018-05-08T05:00:35Z

Someone at Mapbox will need to tag in @mapbox/studio and/or @mapbox/maps-design as this PR includes style spec changes.

asheemmamoowala · 2018-05-08T17:49:35Z

cc @mapbox/studio , @mapbox/maps-design : New raster-scaling raster paint property style-spec addition

andrewharvey · 2018-06-03T17:11:04Z

@mollymerp I'm not sure who's the best person to take a look at this? Not urgent.

lbud

Looks great @andrewharvey! Just a few minor notes 📐 sorry for the delay!

lbud · 2018-06-04T23:43:28Z

src/render/draw_raster.js

+        default:
+            textureFilter = gl.LINEAR;
+            break;
+        }


Since there's only one non-default option for raster-scaling (and paint values that haven't been explicitly defined are initialized as their default value in layer.paint, and we will have already validated enum values), we could condense this block into a line and instantiate it as const:
const textureFilter = layer.paint.get('raster-scaling') === 'nearest' ? gl.NEAREST : gl.LINEAR;

Thanks for the review @lbud. I've simplified this part of the code.

lbud · 2018-06-04T23:57:04Z

src/style-spec/reference/v8.json

+      "doc": "The raster scaling to use for overscaling",
+      "values": {
+        "linear": {
+          "doc": "(Bi)linear filtering"


I'd love to see slightly more descriptive docs for these values —

raster-scaling → "The texture magnification filter to use when overscaling in raster layers."
linear → "Pixels represent the weighted average of the four texels closest to the center of the pixel being textured. The resulting image may be smooth but blurry."
nearest → "Pixels sample from the texel that is nearest to the center of the pixel being sampled. The resulting image may look sharp but pixelated."

(open to better wording! I borrowed some of this from https://www.khronos.org/registry/OpenGL-Refpages/es2.0/xhtml/glTexParameter.xml)

👍 to adding a bit more detail here.

But I'd say there is a strong case for keeping the style spec at a higher level without too much implementation specific details or terminology. Textures and texels are very OpenGL specific.

Fair point — in both linear and nearest then, I think you could replace texel[s] with original source pixel[s] or something like that. I'm personally okay with keeping "texture magnification filter" just in case anyone is looking for the WebGL-specific term, and I think describing both enums' final effects (smooth but blurry/sharp but pixelated) makes it clear enough what it does. But again, open to other options!

Thanks, I've included a bit more detail in the descriptions based on your suggestion.

lbud · 2018-06-05T00:01:22Z

src/style-spec/reference/v8.json

+        }
+      },
+      "default": "linear",
+      "function": "piecewise-constant",


You'll need to rebase this PR to reflect the changes in #6521 (which will replace the function and zoom-function keys here) (sorry for the delay in reviewing!)

Ah, you'll also need to mirror that updated taxonomy in this property — this is what's causing tests to fail. Delete the zoom-function and function keys, and add property-type and expression:

13,14c13,17 < "function": "piecewise-constant", < "zoom-function": true, --- > "property-type": "data-constant", > "expression": { > "interpolated": false, > "parameters": ["zoom"] > },

ah I see now, yeah due to changes which landed after I submitted the PR. Thanks for the pointers.

lbud · 2018-06-05T00:10:45Z

test/integration/render-tests/image/raster-scaling/style.json

+  "version": 8,
+  "metadata": {
+    "test": {
+      "width": 512,


We like to try to keep render tests to 64 x 64px to minimize the time it takes to run the test suite — as long as linear vs nearest results can still be clearly seen at smaller sizes, please reduce the size of these new render tests.

No problem, I've changed this to 64x64, I used 512 simply because all the other tests use this.

mollymerp · 2018-06-05T22:41:21Z

src/style-spec/reference/v8.json

@@ -4392,6 +4392,27 @@
        "data-driven styling": {}
      }
    },
+    "raster-scaling": {


What do you think about renaming this property to raster-filter, raster-mag-filter, or something along those lines so the property name is in line with the existing WebGL terminology?

tool name

carto raster-scaling ref

GIMP interpolation (none (nearest) and linear)

ImageMagick resampling filter

OpenGL texture filtering

gdal resampling method (near, bilinear)

qgis resampling

rasterio resampling method ref

css image-rendering ref

I'm fine with renaming. raster-filter or raster-resampling both seem to be the most widely used.

I don't think terminology necessarily needs to match the underlying implementation's.

I'm leaning more towards raster-resampling due to it's popularity with raster GIS tools, but I'm not fussed either way...

@kkaefer not sure if you saw this thread as well? Resampling and filtering are quite popular too...

…age_source

samanpwbb · 2018-06-06T21:10:02Z

src/style-spec/reference/v8.json

@@ -5013,6 +5013,27 @@
      },
      "property-type": "data-constant"
    },
+    "raster-scaling": {
+      "type": "enum",
+      "doc": "The raster scaling to use for overscaling",


Consider: "The raster scaling interpolation technique to use for overscaling". Is interpolation technique the right word here?

andrewharvey · 2018-06-06T21:22:11Z

Thanks for all the good feedback @lbud @mollymerp @samanpwbb.

kkaefer

How about changing raster-scaling to raster-interpolation? The word scaling feels a little bit out of place here even though that is technically what happens. scaling might invoke the notion of rendering the raster images at a different size, rather than changing the interpolation technique used.

andrewharvey · 2018-06-16T02:37:35Z

test/README.md

@@ -13,7 +13,7 @@ There are two test suites associated with Mapbox GL JS
 To run individual tests:

 - Unit tests: `yarn test-unit path/to/file.test.js` where the path begins within the `/test/unit/` directory
- - Render tests: `yarn test-render render-test-name`
+ - Render tests: `yarn test-render render-test-name` (e.g. `yarn test-render background-color/default`)


@mollymerp I added an example here because it wasn't entirely clear to me what the render-test-name is.

andrewharvey · 2018-06-16T02:57:45Z

@lbud @mollymerp @samanpwbb @kkaefer You've all provided much appreciated feedback on the property name for this, I still don't feel we have a consensus, but given the large variety of terms in use I'm not sure there is a perfect fit. I've changed it now to raster-resampling, but it's not set in stone yet.

tool	name
carto	raster-scaling ref
GIMP	interpolation (none (nearest) and linear)
ImageMagick	resampling filter
OpenGL	texture filtering
gdal	resampling method (near, bilinear)
qgis	resampling
rasterio	resampling method ref
css	image-rendering ref

andrewharvey · 2018-06-18T23:57:14Z

@lbud I believe I've address all your feedback, is it okay from your side or is there anything you're unsure about?

@mollymerp Should this now target 0.47.0 or can it make it into 0.46.0?

mollymerp · 2018-06-19T04:06:51Z

@andrewharvey oof yeah, I think our new release schedule requires this to go into 0.47.0 but you don't need to retarget the branch or anything.

andrewharvey · 2018-06-19T04:11:53Z

@mollymerp I updated the style spec sdk-support version to 0.47.0.

lbud

✨

andrewharvey added the under development 🚧 label Mar 28, 2018

andrewharvey commented Mar 28, 2018

View reviewed changes

andrewharvey mentioned this pull request Mar 28, 2018

Add style-spec option to specify interpolation mode for raster-style sources #4450

Closed

andrewharvey force-pushed the 4450-raster-interpolation branch from c1a040e to 2a8cd04 Compare March 28, 2018 05:47

andrewharvey force-pushed the 4450-raster-interpolation branch from 2a8cd04 to 8f6fb77 Compare April 20, 2018 01:28

andrewharvey removed the under development 🚧 label Apr 20, 2018

andrewharvey changed the title ~~[wip] add raster-scaling raster paint property~~ add raster-scaling raster paint property Apr 20, 2018

andrewharvey mentioned this pull request May 23, 2018

Raster tiles aren't perfectly crisp at integer zoom levels #4552

Closed

lbud suggested changes Jun 5, 2018

View reviewed changes

mollymerp suggested changes Jun 5, 2018

View reviewed changes

andrewharvey added 5 commits June 6, 2018 11:19

add raster-scaling raster paint property

762ea6f

no longer necessary to change default interpolation for texture in im…

9358791

…age_source

add expected render-tests

9145123

target 0.46.0 for support in style-spec docs

bfaac1c

simpler switching

c62f647

andrewharvey force-pushed the 4450-raster-interpolation branch from bdf8e84 to c62f647 Compare June 6, 2018 20:27

samanpwbb approved these changes Jun 6, 2018

View reviewed changes

andrewharvey added the under development 🚧 label Jun 6, 2018

kkaefer reviewed Jun 8, 2018

View reviewed changes

andrewharvey added 3 commits June 12, 2018 08:52

update due to changes since original pr submitted

60d92a8

rename raster-scaling to raster-resampling

fd83002

use 64x64 image render test

e8a68d8

provide example for single render test

efbb31e

andrewharvey commented Jun 16, 2018

View reviewed changes

more details in raster-resampling values

c9ed3e3

andrewharvey removed the under development 🚧 label Jun 16, 2018

andrewharvey changed the title ~~add raster-scaling raster paint property~~ add raster-resampling raster paint property Jun 16, 2018

mollymerp approved these changes Jun 18, 2018

View reviewed changes

mollymerp added feature 🍏 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) labels Jun 18, 2018

push support to 0.47.0

ef58b2a

lbud approved these changes Jun 19, 2018

View reviewed changes

andrewharvey merged commit f546de2 into master Jun 19, 2018

andrewharvey deleted the 4450-raster-interpolation branch June 19, 2018 04:52

mollymerp mentioned this pull request Jun 19, 2018

[core] port raster resampling property mapbox/mapbox-gl-native#12176

Merged

pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 14, 2019

add raster-resampling raster paint property (mapbox#6411)

18b125f

pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 18, 2019

add raster-resampling raster paint property (mapbox#6411)

0031777

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add raster-resampling raster paint property #6411

add raster-resampling raster paint property #6411

andrewharvey commented Mar 28, 2018 •

edited

Loading

andrewharvey Mar 28, 2018

andrewharvey commented Apr 7, 2018

andrewharvey commented Apr 20, 2018

andrewharvey commented Apr 21, 2018

andrewharvey commented May 1, 2018

andrewharvey commented May 8, 2018 •

edited by asheemmamoowala

Loading

asheemmamoowala commented May 8, 2018 •

edited

Loading

andrewharvey commented Jun 3, 2018

lbud left a comment

lbud Jun 4, 2018

andrewharvey Jun 6, 2018

lbud Jun 4, 2018

andrewharvey Jun 6, 2018

lbud Jun 11, 2018

andrewharvey Jun 16, 2018

lbud Jun 5, 2018

andrewharvey Jun 6, 2018

lbud Jun 11, 2018

andrewharvey Jun 11, 2018

lbud Jun 5, 2018

andrewharvey Jun 16, 2018

mollymerp Jun 5, 2018

andrewharvey Jun 6, 2018

andrewharvey Jun 8, 2018

samanpwbb Jun 6, 2018

andrewharvey commented Jun 6, 2018

kkaefer left a comment

andrewharvey Jun 16, 2018

andrewharvey commented Jun 16, 2018

andrewharvey commented Jun 18, 2018

mollymerp commented Jun 19, 2018

andrewharvey commented Jun 19, 2018

lbud left a comment

add raster-resampling raster paint property #6411

add raster-resampling raster paint property #6411

Conversation

andrewharvey commented Mar 28, 2018 • edited Loading

Launch Checklist

Choose a reason for hiding this comment

andrewharvey commented Apr 7, 2018

andrewharvey commented Apr 20, 2018

andrewharvey commented Apr 21, 2018

andrewharvey commented May 1, 2018

andrewharvey commented May 8, 2018 • edited by asheemmamoowala Loading

asheemmamoowala commented May 8, 2018 • edited Loading

andrewharvey commented Jun 3, 2018

lbud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewharvey commented Jun 6, 2018

kkaefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewharvey commented Jun 16, 2018

andrewharvey commented Jun 18, 2018

mollymerp commented Jun 19, 2018

andrewharvey commented Jun 19, 2018

lbud left a comment

Choose a reason for hiding this comment

andrewharvey commented Mar 28, 2018 •

edited

Loading

andrewharvey commented May 8, 2018 •

edited by asheemmamoowala

Loading

asheemmamoowala commented May 8, 2018 •

edited

Loading