Model cache bug introduced in 1.36

[hpinkos]

This code works in 1.35 but not in 1.36

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;
var hpr = new Cesium.HeadingPitchRoll(0, 0, 0);

var origin = Cesium.Cartesian3.fromDegrees(-123, 44, 0);
var modelMatrix = Cesium.Transforms.headingPitchRollToFixedFrame(origin, hpr);
var url = '../../SampleData/models/CesiumMan/Cesium_Man.glb';
var model1 = scene.primitives.add(Cesium.Model.fromGltf({
    url : url,
    modelMatrix : modelMatrix,
    minimumPixelSize : 128
}));
model1.readyPromise.then(function() {
    var model2 = Cesium.Model.fromGltf({
        url : url,
        modelMatrix : modelMatrix,
        minimumPixelSize : 128
    });
    scene.primitives.removeAll();
    scene.primitives.add(model2);
});

As far as I can tell, model2 gets it's glTF from cache, then context.cache.modelRendererResourceCache is cleared when model1 is removed, and that causes model2 to lose track of it's buffer and this error message shows up because eventually model.gltf.buffers[id].extras._pipeline.source = buffer; gets set to an arraybuffer of 0 length.

DeveloperError: sub-region exceeds array bounds.
Error
    at new DeveloperError (http://cesiumjs.org/releases/1.36/Source/Core/DeveloperError.js:43:19)
    at getStringFromTypedArray (http://cesiumjs.org/releases/1.36/Source/Core/getStringFromTypedArray.js:27:19)
    at http://cesiumjs.org/releases/1.36/Source/Scene/Model.js:1480:30
    at Function.ForEach.object (http://cesiumjs.org/releases/1.36/Source/ThirdParty/GltfPipeline/ForEach.js:17:35)
    at Function.ForEach.topLevel (http://cesiumjs.org/releases/1.36/Source/ThirdParty/GltfPipeline/ForEach.js:30:17)
    at Function.ForEach.shader (http://cesiumjs.org/releases/1.36/Source/ThirdParty/GltfPipeline/ForEach.js:174:17)
    at parseShaders (http://cesiumjs.org/releases/1.36/Source/Scene/Model.js:1473:17)
    at Model.update (http://cesiumjs.org/releases/1.36/Source/Scene/Model.js:4552:25)
    at PrimitiveCollection.update (http://cesiumjs.org/releases/1.36/Source/Scene/PrimitiveCollection.js:365:27)
    at updatePrimitives (http://cesiumjs.org/releases/1.36/Source/Scene/Scene.js:2441:27)

[hpinkos]

@lilleyse @moneimne

[mramato]

CC @pjcozzi

Since this is breaking Composer, I took a quick look and tracked it down to CachedRendererResources and what looks like fundamentally broken caching. The end result is that both models have their own instance of CachedRendererResources and when the first model is removed, it calls CachedRendererResources.release and destroys the shared resources that both instances point to.

The crux of the issue is that there are 2 caches, the gltfCache that's local to the Model.js module and the modelRendererResourceCache that's on context.cache. (I assume the later is on the context because we can't share webgl resources across contexts.)

The bug manifests when the Model constructor checks if an item is in gltfCache and gets a hit, at that point it increments the cache count, finishes initialization and returns. This is a bug because modelRendererResourceCache does not get updated until Model.update is called, which means that until the next frame the two caches are out of sync. In @hpinkos's code, when model1 is removed, the gltfCache count goes from 2 to 1 as expected, but the modelRendererResourceCache goes from 1 to 0 and is deleted because Model.update hasn't been called for model2 yet (nor should it have to be).

The fix here is to not allow the caches to get out of sync and update the count on modelRendererResourceCache from inside the constructor if it already exists. However, the Model constructor does not require a scene object and otherwise does not have access to the context, which means it's impossible to increment the count. So this is an architectural bug more than anything else.

I think the best solution may be to delay any and all Model initialization until it's added to a scene (and update is called the first time). This allows us access to the context without a breaking API change and keep all caches in sync. But I didn't write any of this code so maybe you guys have a better idea.

Either way, this should definitely get fixed ASAP.

[pjcozzi]

Thanks for researching this. It probably does require delaying things to update, which is pretty widely done throughout the primitives. It should also play nice with 3D Tiles.

@lilleyse can you please make this change since it has potential subtle impact? Let's try to get it into master soon.