Fix Material caching issue

[mramato]

Run the below code in Sandcastle, hit the first button, wait for it to load, then his the second button. In master the second polygon will be gray, in this branch both polygons will have the proper texture. The problem is that in master, neither the _loadedImages or _loadedCubeMaps arrays are being properly populated when the image was already in the texture cache. In order to fix this I had to keep the source images around in the cache as well and add them to the proper collection when a cache hit occurs.

There also appears to be a race condition in Material, which I'll write up a second issue for. Basically if you hit the buttons below quickly (or just add them both in the same function instead of having 2 buttons) everything works. This is because Material actually loads the same image twice, rather than keeping a promise to the image from the first request. Looking at the Material code, there are most likely additional bugs as well, we should really look into refactoring the entire object.

Cesium.Material._materialCache.addMaterial('Wallpaper', {
    fabric : {
        type : 'Wallpaper',
        uniforms : {
            image : Cesium.Material.DefaultImageId,
            anchor : new Cesium.Cartesian2(0, 0)
        },
        components : {
            diffuse : 'texture2D(image, fract((gl_FragCoord.xy - anchor.xy) / vec2(imageDimensions.xy))).rgb',
            alpha : 'texture2D(image, fract((gl_FragCoord.xy - anchor.xy) / vec2(imageDimensions.xy))).a'
        }
    },
    translucent : false
});

var WallPaperMaterialProperty = function(scene, image, anchor) {
    this._scene = scene;
    this._image = image;
    this._anchor = anchor;
    this.definitionChanged = new Cesium.Event();
    this.isConstant = true;
};

WallPaperMaterialProperty.prototype.getType = function(time) {
    return 'Wallpaper';
};

WallPaperMaterialProperty.prototype.getValue = function(time, result) {
    if (!Cesium.defined(result)) {
        result = {
            image : undefined,
            anchor : undefined
        };
    }
    result.image = this._image;
    result.anchor = Cesium.SceneTransforms.wgs84ToDrawingBufferCoordinates(this._scene, this._anchor, result.anchor);
    if (Cesium.defined(result.anchor)) {
        result.anchor.x = Math.floor(result.anchor.x);
        result.anchor.y = Math.floor(result.anchor.y);
    } else {
        result.anchor = new Cesium.Cartesian2(0, 0);
    }
    return result;
};

WallPaperMaterialProperty.prototype.equals = function(other) {
    return this === other || //
    (other instanceof WallPaperMaterialProperty && //
    this._image === other._image);
};

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

Sandcastle.addToolbarButton('Test1', function() {
    var customDataSource1 = new Cesium.CustomDataSource("testing");
    viewer.dataSources.add(customDataSource1);
    var customRectangle = customDataSource1.entities.add({
        id : "rect1",
        rectangle : {
            coordinates : Cesium.Rectangle.fromDegrees(-119, 44, -112, 47),
            material : new WallPaperMaterialProperty(viewer.scene, "../images/checkerboard.png", Cesium.Cartesian3.fromDegrees(-119, 44))
        }
    });

});

Sandcastle.addToolbarButton('Test2', function() {
    var customRectangle2 = viewer.entities.add({
        id : "rect2",
        rectangle : {
            coordinates : Cesium.Rectangle.fromDegrees(-110.0, 20.0, -80.0, 25.0),
            material : new WallPaperMaterialProperty(viewer.scene, "../images/checkerboard.png", Cesium.Cartesian3.fromDegrees(-110, 20))
        }
    });
});

[mramato]

I'll also add that because of the race condition and current limitation of the Material class to only work with image urls, it looked impossible to add a test for this, though I'm open to suggestions if anyone has one that doesn't involve even more changes to the Material object for now.

[pjcozzi]

Thanks @mramato. I'll look at this next week.

[pjcozzi]

Keeping image objects at this level of the engine is something we want to avoid.

I plan on doing a system-wide texture cache/load pipeline soon so let's hold on this since it will replace it.

[mramato]

How soon? There are users hitting this bug now and this seems like a simple interim fix. I agree that not having to cache these is the right solution, but if it's 3 months out, then I'm not sure what harm in doing this in the short term is (unless there are existing use cases where memory really does become an issue because of the cache). Ultimately it's up to you, just close this if you don't want it merged.

[pjcozzi]

Gunning for 1.10. We could also leave this open and make a decision mid-to-late May.

[mramato]

Sounds fine with me.

[mramato]

I plan on doing a system-wide texture cache/load pipeline soon so let's hold on this since it will replace it.

I assume this won't happened for 1.10? Will it definitely for 1.11? We can continue to hold off on this, I just wanted to know where we are at.

[pjcozzi]

Yes, 1.11 is likely.

[mramato]

I plan on doing a system-wide texture cache/load pipeline soon so let's hold on this since it will replace it.

@pjcozzi I assume this isn't happening for 1.11? Should I update this branch so we can fix this issue in the short term, it's cropped up a few times on the forum.

[pjcozzi]

If I recall, this requires keeping a copy of each texture in CPU memory, right? This seems bad for those streaming textured buildings with custom geometries and Cesium materials, for example.

Can you link to all the forum issues in this issue, and I'll have a look at them?

[mramato]

The most recent one is: https://groups.google.com/d/msg/cesium-dev/Qhos2CpA1cY/Q2X_jTNDktUJ

But the original is: https://groups.google.com/d/msg/cesium-dev/laVtluz3VRY/qjHdbUQ2Uj8J

You are correct that we have to keep the texture around, but our approach to image based uniforms seems fundamentally broken otherwise, since you can never use the same texture more than once.

Another possible solution would be to remove the Material texture cache completely, since I think that's where the problem lies. This should fix the bug but also prevent us from keeping around the source in memory. I'm not sure what benefit the cache provides in its current state.

[pjcozzi]

Another possible solution would be to remove the Material texture cache completely, since I think that's where the problem lies. This should fix the bug but also prevent us from keeping around the source in memory. I'm not sure what benefit the cache provides in its current state.

Sounds like the best solution for now. I can certainty fix these problems for real, but I can't commit to a timeframe. This would also replace #2843, right?

[mramato]

Superseded by #2850

[mramato]

Yes, it replaces #2843 as well and also fixes a few new issues I uncovered in the process.