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.

Sign up for GitHub

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

Jump to bottom

Fix rendering images with nearest sampling and 2^n size #11162

Merged
merged 13 commits into from
Oct 27, 2021
Merged

Fix rendering images with nearest sampling and 2^n size #11162

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b7bb196
Added render tests for image with a power of 2 size
SnailBones Oct 22, 2021
73b2487
Added nearest resampling, test is failing
SnailBones Oct 22, 2021
49f45e0
Fix by disabling mipmaps
SnailBones Oct 22, 2021
bebd455
Keeping same behavior with linear sampling
SnailBones Oct 22, 2021
0dcfc2c
Correcting expected.ping to use nearest filtering
SnailBones Oct 26, 2021
19896ce
Adding check for useMipMap
SnailBones Oct 26, 2021
57d18b8
Refactoring
SnailBones Oct 26, 2021
0b81246
Removing third argument from
SnailBones Oct 26, 2021
69dbf01
Fix broken refactoring
SnailBones Oct 26, 2021
976b8e1
Removing stray console.log
SnailBones Oct 26, 2021
f21aba4
Readd mipmaps
SnailBones Oct 26, 2021
144e4c6
minor refactor
SnailBones Oct 26, 2021
c98c5a9
Mipmaps with nearest
SnailBones Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/render/draw_raster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,17 @@ function drawRaster(painter: Painter, sourceCache: SourceCache, layer: RasterSty
const textureFilter = layer.paint.get('raster-resampling') === 'nearest' ? gl.NEAREST : gl.LINEAR;

context.activeTexture.set(gl.TEXTURE0);
tile.texture.bind(textureFilter, gl.CLAMP_TO_EDGE, gl.LINEAR_MIPMAP_NEAREST);
tile.texture.bind(textureFilter, gl.CLAMP_TO_EDGE);

context.activeTexture.set(gl.TEXTURE1);

if (parentTile) {
parentTile.texture.bind(textureFilter, gl.CLAMP_TO_EDGE, gl.LINEAR_MIPMAP_NEAREST);
parentTile.texture.bind(textureFilter, gl.CLAMP_TO_EDGE);
parentScaleBy = Math.pow(2, parentTile.tileID.overscaledZ - tile.tileID.overscaledZ);
parentTL = [tile.tileID.canonical.x * parentScaleBy % 1, tile.tileID.canonical.y * parentScaleBy % 1];

} else {
tile.texture.bind(textureFilter, gl.CLAMP_TO_EDGE, gl.LINEAR_MIPMAP_NEAREST);
tile.texture.bind(textureFilter, gl.CLAMP_TO_EDGE);
}

const uniformValues = rasterUniformValues(projMatrix, parentTL || [0, 0], parentScaleBy || 1, fade, layer);
Expand Down
15 changes: 5 additions & 10 deletions src/render/texture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,18 +52,16 @@ class Texture {

update(image: TextureImage, options: ?{premultiply?: boolean, useMipmap?: boolean}, position?: { x: number, y: number }) {
const {width, height} = image;
const resize = (!this.size || this.size[0] !== width || this.size[1] !== height) && !position;
const {context} = this;
const {gl} = context;

this.useMipmap = Boolean(options && options.useMipmap);
gl.bindTexture(gl.TEXTURE_2D, this.texture);

context.pixelStoreUnpackFlipY.set(false);
context.pixelStoreUnpack.set(1);
context.pixelStoreUnpackPremultiplyAlpha.set(this.format === gl.RGBA && (!options || options.premultiply !== false));

if (resize) {
if (!position && (!this.size || this.size[0] !== width || this.size[1] !== height)) {
this.size = [width, height];

if (image instanceof HTMLImageElement || image instanceof HTMLCanvasElement || image instanceof HTMLVideoElement || image instanceof ImageData || (ImageBitmap && image instanceof ImageBitmap)) {
Expand All @@ -81,23 +79,20 @@ class Texture {
}
}

if (this.useMipmap && this.isSizePowerOfTwo()) {
this.useMipmap = Boolean(options && options.useMipmap && this.isSizePowerOfTwo());
if (this.useMipmap) {
gl.generateMipmap(gl.TEXTURE_2D);
}
}

bind(filter: TextureFilter, wrap: TextureWrap, minFilter: ?TextureFilter) {
bind(filter: TextureFilter, wrap: TextureWrap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on removing the extra parameter as it really simplifies use of that class (basically infers that from the option request to use mipmaps).

const {context} = this;
const {gl} = context;
gl.bindTexture(gl.TEXTURE_2D, this.texture);

if (minFilter === gl.LINEAR_MIPMAP_NEAREST && !this.isSizePowerOfTwo()) {
minFilter = gl.LINEAR;
}

if (filter !== this.filter) {
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, filter);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, minFilter || filter);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, (this.useMipmap && filter === gl.LINEAR) ? gl.LINEAR_MIPMAP_NEAREST : filter);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 That's the correct fallback a filtering request on LINEAR.

For NEAREST, if we request texture.bind(NEAREST, ...) I would still expect as a user of this class to fallback to NEAREST_MIPMAP_NEAREST (nearest within the mip level and nearest between mip levels).

It seems that most of our use cases need to stay non-linear between mip level as a default (we're not using trilinear, but we could extend that when we need it for some cases, e.g. LINEAR_MIPMAP_LINEAR), so we can stick to that for now but still hint the requested filter for within the mip (e.g. filter_MIPMAP_NEAREST).

Copy link
Contributor

Choose a reason for hiding this comment

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

In practice what that means as a fallback in this case:

  • LINEAR + mipmap -> LINEAR_MIPMAP_NEAREST
  • NEAREST + mipmap -> NEAREST_MIPMAP_NEAREST

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Happy to add the condition for nearest with mipmap. In practice I don't think this is ever happening, but not a bad idea to future-proof it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a possible combination when a user chooses nearest as a paint properties with raster-resampling because we always request mipmaps for it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! I noticed that code path didn't run in my example, but it should now correctly use mipmaps when it is called. (Repeating textures?)

this.filter = filter;
}

Expand Down
2 changes: 1 addition & 1 deletion src/source/raster_tile_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class RasterTileSource extends Evented implements Source {
tile.texture.update(img, {useMipmap: true});
} else {
tile.texture = new Texture(context, img, gl.RGBA, {useMipmap: true});
tile.texture.bind(gl.LINEAR, gl.CLAMP_TO_EDGE, gl.LINEAR_MIPMAP_NEAREST);
tile.texture.bind(gl.LINEAR, gl.CLAMP_TO_EDGE);

if (context.extTextureFilterAnisotropic) {
gl.texParameterf(gl.TEXTURE_2D, context.extTextureFilterAnisotropic.TEXTURE_MAX_ANISOTROPY_EXT, context.extTextureFilterAnisotropicMax);
Expand Down
2 changes: 1 addition & 1 deletion src/terrain/draw_terrain_raster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ function drawTerrainRaster(painter: Painter, terrain: Terrain, sourceCache: Sour

// Bind the main draped texture
context.activeTexture.set(gl.TEXTURE0);
tile.texture.bind(gl.LINEAR, gl.CLAMP_TO_EDGE, gl.LINEAR_MIPMAP_NEAREST);
tile.texture.bind(gl.LINEAR, gl.CLAMP_TO_EDGE);

const morph = vertexMorphing.getMorphValuesForProxy(coord.key);
const shaderMode = morph ? SHADER_MORPHING : SHADER_DEFAULT;
Expand Down
4 changes: 2 additions & 2 deletions src/terrain/terrain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,12 +576,12 @@ export class Terrain extends Elevation {
context.activeTexture.set(gl.TEXTURE2);
const demTexture = this._prepareDemTileUniforms(tile, demTile, uniforms) ?
(demTile.demTexture: any) : this.emptyDEMTexture;
demTexture.bind(gl.NEAREST, gl.CLAMP_TO_EDGE, gl.NEAREST);
demTexture.bind(gl.NEAREST, gl.CLAMP_TO_EDGE);
}

if (options && options.useDepthForOcclusion) {
context.activeTexture.set(gl.TEXTURE3);
this._depthTexture.bind(gl.NEAREST, gl.CLAMP_TO_EDGE, gl.NEAREST);
this._depthTexture.bind(gl.NEAREST, gl.CLAMP_TO_EDGE);
uniforms['u_depth_size_inv'] = [1 / this._depthFBO.width, 1 / this._depthFBO.height];
}

Expand Down
Binary file added test/integration/image/256.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added test/integration/render-tests/image/power-of-two/expected.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
29 changes: 29 additions & 0 deletions test/integration/render-tests/image/power-of-two/style.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"version": 8,
"metadata": {
"test": {
"width": 256,
"height": 256
}
},
"center": [34.5, 54.5],
"zoom": 6,
"sources": {
"image": {
"type": "image",
"coordinates": [[34, 55], [35, 55], [35, 54], [34, 54]],
"url": "local://image/256.png"
}
},
"layers": [
{
"id": "image",
"type": "raster",
"source": "image",
"paint": {
"raster-fade-duration": 0,
"raster-resampling": "nearest"
}
}
]
}