Set texture sampler properties of ImageryLayer

[forman]

This is ready and addresses #5846, Nearest-neighbor image resampling for ImageryLayer.

It adds two new properties to ImageryLayer:

• minificationFilter with possible values TextureMinificationFilter.LINEAR and .NEAREST
• magnificationFilter with possible values TextureMagnificationFilter.LINEAR and .NEAREST

[cesium-concierge]

Welcome to the Cesium community @forman!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

I am a bot who helps you make Cesium awesome! Thanks again.

[forman]

CLA has been sent just now.

[pjcozzi]

Thanks for the contribution, @forman! We received your CLA!

[pjcozzi]

Can you please make TextureMinificationFilter and TextureMagnificationFilter public and add reference doc for them and each enum value? Right now the new properties technically reference these private enums.

Also mention these as new in CHANGES.md please.

[pjcozzi]

@lilleyse can you review this?

[lilleyse]

@lilleyse can you review this?

Sure, I should be able to get to this tomorrow.

[forman]

@pjcozzi, @lilleyse

Can you please make TextureMinificationFilter and TextureMagnificationFilter public

Done.

and add reference doc for them and each enum value

Done naively, because I couldn't find proper description of WebGL NEAREST|LINEAR__MIPMAP_NEAREST|LINEAR constants.

In addition, I'd like some feedback:

• long names for the new ImageryLayer properties may be clearer, e.g. textureMagFilter or textureMagnificationFilter instead of magnificationFilter, what do you think?
• the new ImageryLayer properties should be validated. Not sure how and where to best utilize the validate(f) method of TextureMinificationFilter, TextureMagnificationFilter. I guess I'd have to explicitly add them to defineProperties(ImageryLayer.proptotype) and provide a setter? Or is there some automatism built into Cesium? I'd also like to add a test for the validation.

[lilleyse]

Usually we don't use line breaks in CHANGES.md, one long line is fine.

Hence, the enums `TextureMagnificationFilter` and `TextureMinificationFilter` have been made public.

can be its own bullet.

[lilleyse]

We typically make the type uppercase Boolean.

[lilleyse]

validate can be marked as @private.

[lilleyse]

Same comments for this section as above.

[lilleyse]

For consistency with the rest of the code, DEFAULT_MINIFICATION_FILTER should be above ImageryLayer.DEFAULT_MAGNIFICATION_FILTER

[lilleyse]

This can just be new ImageryLayer(provider);

[lilleyse]

If you decide to leave texture filtering out of the imagery provider, tweak this and below's doc.

[lilleyse]

It should be noted somewhere that these values can be set immediately after adding the imagery layer but once a texture is loaded it won't be possible to change its filter.

[lilleyse]

Use @exports TextureMagnificationFilter instead, so it appears as its own section in the documentation. Same for TextureMinificationFilter.

[lilleyse]

For each of these also add

         * @type {Number}
         * @constant

[lilleyse]

Do you have some sample code for this? I'm having some problems with seeing nearest filtering, even though I see it gets to the sampler correctly. Maybe I'm doing something incorrectly:

var viewer = new Cesium.Viewer('cesiumContainer');

var layers = viewer.imageryLayers;
layers.removeAll();

var blackMarbleLinear = layers.addImageryProvider(Cesium.createTileMapServiceImageryProvider({
    url : 'https://cesiumjs.org/blackmarble',
    credit : 'Black Marble imagery courtesy NASA Earth Observatory',
    flipXY : true // Only old gdal2tile.py generated tilesets need this flag.
}));

var blackMarbleNearest = layers.addImageryProvider(Cesium.createTileMapServiceImageryProvider({
    url : 'https://cesiumjs.org/blackmarble',
    credit : 'Black Marble imagery courtesy NASA Earth Observatory',
    flipXY : true // Only old gdal2tile.py generated tilesets need this flag.
}));

blackMarbleNearest.splitDirection = Cesium.ImagerySplitDirection.LEFT; // Only show to the left of the slider.
blackMarbleNearest.minificationFilter = Cesium.TextureMinificationFilter.NEAREST;
blackMarbleNearest.magificationFilter = Cesium.TextureMagnificationFilter.NEAREST;

// Sync the position of the slider with the split position
var slider = document.getElementById('slider');
viewer.scene.imagerySplitPosition = (slider.offsetLeft) / slider.parentElement.offsetWidth;

var dragStartX = 0;

document.getElementById('slider').addEventListener('mousedown', mouseDown, false);
window.addEventListener('mouseup', mouseUp, false);

function mouseUp() {
  window.removeEventListener('mousemove', sliderMove, true);
}

function mouseDown(e) {
  var slider = document.getElementById('slider');
  dragStartX = e.clientX - slider.offsetLeft;
  window.addEventListener('mousemove', sliderMove, true);
}

function sliderMove(e) {
  var slider = document.getElementById('slider');
  var splitPosition = (e.clientX - dragStartX) / slider.parentElement.offsetWidth;
  slider.style.left = 100.0 * splitPosition + "%";
  viewer.scene.imagerySplitPosition = splitPosition;
}

[lilleyse]

Done naively, because I couldn't find proper description of WebGL NEAREST|LINEAR__MIPMAP_NEAREST|LINEAR constants.

Those descriptions are fine.

long names for the new ImageryLayer properties may be clearer, e.g. textureMagFilter or textureMagnificationFilter instead of magnificationFilter, what do you think?

I'm okay with the names as they are.

the new ImageryLayer properties should be validated. Not sure how and where to best utilize the validate(f) method of TextureMinificationFilter, TextureMagnificationFilter. I guess I'd have to explicitly add them to defineProperties(ImageryLayer.proptotype) and provide a setter? Or is there some automatism built into Cesium? I'd also like to add a test for the validation.

The validation is handled when the sampler is created for the texture, so I don't think anything extra needs to be added.

[forman]

@lilleyse thanks for the review, Fixem them now. Have an idea. why the example wont work...

[forman]

@lilleyse the example doesn't work for at least two reasons:

1. For a given Cesium frameState instance only two Sampler objects can be cached in the current ImageryLayer implementation: context.cache.imageryLayer_mipmapSampler and context.cache.imageryLayer_nonMipmapSampler. For the PR to work we need different samplers per ImageryLayer instance and therefore I had change the ImageryLayer to use keyed sample cache; see eb5ec0c.
2. It seems that the texture filters have no effect on mipmap textures. At least this could explain why I get NEAREST for the imagery layers in my application but still LINEAR (or so) for all other standard Cesium imagery layers, e.g. Natural Earth II. My imagery layers originate from variables in NetCDF files and usually have tile sizes like 270 or 360.

If my theory holds, your example should work with non-mipmap textures. (And after you fix a tiny typo: magificationFilter --> magnificationFilter.)

I fear, I need some more (Web)GL insights to make this work for all kind of imagery layers. I'd be really glad if someone could help here, first to get this PR done and secondly because we urgently need this feature in our app.

Thanks

EDIT:

I believe, this line has no effect.

[forman]

@lilleyse If I comment out creation of mipmaps in ImageryLayer, your example will work as expected:

Here the SandCastle code:

var viewer = new Cesium.Viewer('cesiumContainer');

var layers = viewer.imageryLayers;
layers.removeAll();


var layerLinear = layers.addImageryProvider(Cesium.createTileMapServiceImageryProvider({
    url : require.toUrl('Assets/Textures/NaturalEarthII')
}));

var layerNearest = layers.addImageryProvider(Cesium.createTileMapServiceImageryProvider({
    url : require.toUrl('Assets/Textures/NaturalEarthII')
}));

layerNearest.splitDirection = Cesium.ImagerySplitDirection.RIGHT;
layerNearest.minificationFilter = Cesium.TextureMinificationFilter.NEAREST;
layerNearest.magnificationFilter = Cesium.TextureMagnificationFilter.NEAREST;

var slider = document.getElementById('slider');
viewer.scene.imagerySplitPosition = (slider.offsetLeft) / slider.parentElement.offsetWidth;

var dragStartX = 0;

document.getElementById('slider').addEventListener('mousedown', mouseDown, false);
window.addEventListener('mouseup', mouseUp, false);

function mouseUp() {
  window.removeEventListener('mousemove', sliderMove, true);
}

function mouseDown(e) {
  var slider = document.getElementById('slider');
  dragStartX = e.clientX - slider.offsetLeft;
  window.addEventListener('mousemove', sliderMove, true);
}

function sliderMove(e) {
  var slider = document.getElementById('slider');
  var splitPosition = (e.clientX - dragStartX) / slider.parentElement.offsetWidth;
  slider.style.left = 100.0 * splitPosition + "%";
  viewer.scene.imagerySplitPosition = splitPosition;
}

[forman]

This is ready now for another review.

• Added Sandcastle example "Imagery Layers Texture Filters"
• Changed ImageryLayer.finalizeReprojectTexture() which now requires special attention w.r.t. potential runtime performance loss (well, which I can't recognize).

[lilleyse]

We usually try to be more explicit about checking if something is undefined. The section could be written slightly differently to be a bit more conformant with the rest of the code:

var mipmapSamplers = context.cache.imageryLayerMipmapSamplers;
if (!defined(mipmapSamplers)) {
    mipmapSamplers = {};
    context.cache.imageryLayerMipmapSamplers = mipmapSamplers;
}
var mipmapSampler = mipmapSamplers[mipmapSamplerKey];
if (!defined(mipmapSampler)) {
    mipmapSampler = new Sampler({
        wrapS : TextureWrap.CLAMP_TO_EDGE,
        wrapT : TextureWrap.CLAMP_TO_EDGE,
        minificationFilter : minificationFilter,
        magnificationFilter : magnificationFilter,
        maximumAnisotropy : maximumAnisotropy
    });
    mipmapSamplers[mipmapSamplerKey] = mipmapSampler;
}

[forman]

Will change this, thanks!

[lilleyse]

The check for NEAREST here is no longer needed.

[forman]

oops

[lilleyse]

Changed ImageryLayer.finalizeReprojectTexture() which now requires special attention w.r.t. potential runtime performance loss (well, which I can't recognize).

The code doesn't seem too much of a concern to me. I ran the Chrome profiler in case and didn't notice any change.

[lilleyse]

All else looks good, this should be ready soon.

[forman]

Done. May add an explicit spec later verifying ImageryLayer._reprojectTexture creates the expected samplers, however, current tests seem to cover code already.

[lilleyse]

I tweaked some wording in the recent commit.

May add an explicit spec later verifying ImageryLayer._reprojectTexture creates the expected samplers, however, current tests seem to cover code already.

This would be good to add. I think just editing the existing test to check imageryLayer.texture.sampler would work fine.

[forman]

Ready now.

[lilleyse]

The failing test is unrelated to this PR, so this should be good to go.

Thanks @forman!

[pjcozzi]

@lilleyse this @see - and the one in TextureMinificationFilter.js - is not needed and not customary. Can you please remove in master?

[pjcozzi]

@lilleyse are these tests sufficient? How is coverage? Should these also do a smokescreen render?

[pjcozzi]

This is an important comment since I do not believe we ever create mipmaps for imagery tiles since the mipmapping is basically implicit in the tileset. Is this ever validated? A DeveloperError should be thrown if one of these two values is not set.

[pjcozzi]

In this file and below, try to avoid providing reference doc that is just a repeat of the enum. @lilleyse could you add a tad more description and trade offs, e.g., visual quality vs speed?