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

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 5 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/render/draw_raster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,22 @@ function drawRaster(painter: Painter, sourceCache: SourceCache, layer: RasterSty

let parentScaleBy, parentTL;

const textureFilter = layer.paint.get('raster-resampling') === 'nearest' ? gl.NEAREST : gl.LINEAR;
const nearest = layer.paint.get('raster-resampling') === 'nearest';
const textureFilter = nearest ? gl.NEAREST : gl.LINEAR;
const minFilter = nearest ? gl.NEAREST : gl.LINEAR_MIPMAP_NEAREST;

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, minFilter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we fixing this at the correct level, have you considered whether there is something incorrect within the internal texture class?

I feel like the texture class could be safer for the developer based on intent. At the moment it's possible to call texture.bind(filter, a_mip_map_filter), without necessarily having created the texture with a call that falls through generateMipMap(), resulting in an incoherent mipmap filter set on a texture that doesn't have any mipmap created.

We could assert or warn if from the callee we request that combination to prevent silent failure.

The flag this.useMipmap in that class may be misleading, because it may be set to true, but might not represent the internal state of whether that texture uses mipmaps. I'd suggest replacing it with this.canUseMipMaps = Boolean(options && options.useMipmap) && isPowerOfTwo().

Then, at binding:

bind(...): 
   if (minFilter is mipmap filter && !canUseMipMaps) {
      incoherent filter request: no mipmap created and requested a mipmap filter, fallback to minFilter = filter
   }
   ...


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, minFilter);
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, minFilter);
}

const uniformValues = rasterUniformValues(projMatrix, parentTL || [0, 0], parentScaleBy || 1, fade, layer);
Expand Down
Binary file added test/integration/image/256.png
View file
Open in desktop
Loading
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
Loading
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"
}
}
]
}