Fix handling image/file cache for three r178 and above

[vincentfretin]

Fix handling image/file cache now that the ImageLoader/FileLoader are using a prefix since three r178 (fix #5771)

The img tags in a-assets are loaded via the load event and set in THREE.Cache, later if the same image is loaded via its url instead of the id, then the material system uses ImageLoader to load that image and it will be already in the cache.
For a glb loaded via a-asset-item tag, it set the file in THREE.Cache, when using glf-model with that asset id or the same src, GLTFLoader will use FileLoader that will take the file from the cache.

[vincentfretin]

For this code path, I'm not sure how to trigger an example using this cache so if "file:" prefix is correct here or if it should be "image:"?, using autoplay preload="auto" on a video I didn't manage to trigger that code and no tests are failing for that change.

[vincentfretin]

For video, the material system doesn't go through FileLoader or ImageLoader, just using the existing video element or creating one. Setting the video element in the cache is useless here, it's used nowhere. Also with a good connection, we hit el.readyState === 4 right away and the promise resolve without settting the cache anyway.
For mobile (simulate with Fast 4G in Chrome), it only downloads a few seconds 11.537 over the total 137.44 and the video.readyState becomes 4, and the assets timeout... (with timeout="30000").
So I'll completely remove the line.

[vincentfretin]

Note that setting the cache in the test was wrong in my opinion, that didn't test that we set the cache for img added in a-assets. I added a comment in the changes.

[vincentfretin]

@mrxz I believe the changes are correct but please double check.

[mrxz]

The image element should only be added to the Cache in case the complete flag is true. Otherwise, there's the possible (albeit unlikely) scenario that the ImageLoader encounters it while still incomplete, which would cause it to wait in an incompatible way, never resolving.

This would require the asset loading timeout to be hit, but it's precisely this scenario that might benefit from a cache-miss.

[vincentfretin]

I tried to produce this case with a modified examples/boilerplate/panorama/index.html

      <a-assets timeout="3000">
        <img src="puydesancy.jpg" crossorigin="anonymous">
      </a-assets>
      <a-sky src="puydesancy.jpg" rotation="0 -130 0"></a-sky>

the example is bit silly because in a real case you would have used

      <a-assets timeout="3000">
        <img id="puydesancy" src="puydesancy.jpg" crossorigin="anonymous">
      </a-assets>
      <a-sky src="#puydesancy" rotation="0 -130 0"></a-sky>

but this second snippet won't go in ImageLoader, only the first one.

First the timeout feature is currently not quite working for images probably since 0d97218 because we're waiting for readyState complete (meaning DOM, stylesheets and images are loaded), so we actually create the setTimeout for the timeout feature after we loaded all images, some a-asset-item may still be loading though so the timeout feature is not completely broken. So first fix is to wait for readyState interactive (dom loaded only) or complete only for a-assets.

diff --git a/src/core/a-assets.js b/src/core/a-assets.js
index 89a38a1f..609f40a0 100644
--- a/src/core/a-assets.js
+++ b/src/core/a-assets.js
@@ -17,6 +17,24 @@ class AAssets extends ANode {
     this.timeout = null;
   }
 
+  /**
+   * Override connectedCallback to initialize at 'interactive' instead of 'complete'.
+   * This allows the timeout mechanism to work - if we wait for 'complete', all resources
+   * (including images) are already loaded, defeating the purpose of the timeout.
+   */
+  connectedCallback () {
+    var self = this;
+    if (document.readyState === 'interactive' || document.readyState === 'complete') {
+      this.doConnectedCallback();
+      return;
+    }
+    document.addEventListener('readystatechange', function onReadyStateChange () {
+      if (document.readyState !== 'interactive' && document.readyState !== 'complete') { return; }
+      document.removeEventListener('readystatechange', onReadyStateChange);
+      self.doConnectedCallback();
+    });
+  }
+
   doConnectedCallback () {
     var self = this;
     var i;

and also showing the loading screen earlier in readyState interactive so we don't stare at a white page. I'll do a PR for that.

Then we can indeed hit the timeout, but even with the timeout, all components initialization wait for readyState complete so actually all images are downloaded before any ImageLoader.load call by a material component, so triggering the case of loading the same image in ImageLoader while the img in a-assets is still downloading is not possible from what I can tell.
Moving the Cache.add when img is complete like this

@@ -41,12 +60,15 @@ class AAssets extends ANode {
       loaded.push(new Promise(function (resolve, reject) {
         // Set in cache because we won't be needing to call three.js loader if we have.
         // a loaded media element.
-        THREE.Cache.add('image:' + imgEls[i].getAttribute('src'), imgEl);
         if (imgEl.complete) {
+          THREE.Cache.add('image:' + imgEls[i].getAttribute('src'), imgEl);
           resolve();
           return;
         }
-        imgEl.onload = resolve;
+        imgEl.onload = function() {
+          THREE.Cache.add('image:' + imgEls[i].getAttribute('src'), imgEl);
+          resolve();
+        }
         imgEl.onerror = reject;
       }));
     }

make the test fail because the callback materialtextureloaded is executed before the img onload, a simple setTimeout won't do, a setTimeout 10 is working but not great

    test('does not invoke XHR if passing <img>', function (done) {
      var assetsEl = document.createElement('a-assets');
      var img = document.createElement('img');
      var imageLoaderSpy = this.sinon.spy(THREE.ImageLoader.prototype, 'load');
      var textureLoaderSpy = this.sinon.spy(THREE.TextureLoader.prototype, 'load');
      img.setAttribute('src', IMG_SRC);
      img.setAttribute('id', 'foo');
      THREE.Cache.clear();
      assetsEl.appendChild(img);
      el.sceneEl.appendChild(assetsEl);
      // Adding the asset will add image:${IMG_SRC} in THREE.Cache
      // without going through THREE.ImageLoader
      el.addEventListener('materialtextureloaded', function () {
        assert.notOk(imageLoaderSpy.called);
        assert.notOk(textureLoaderSpy.called);
        setTimeout(() => {
          assert.equal(THREE.Cache.get(`image:${IMG_SRC}`), img);
          THREE.Cache.clear();
          THREE.ImageLoader.prototype.load.restore();
          THREE.TextureLoader.prototype.load.restore();
          done();
        }, 10);
      });
      el.setAttribute('material', 'src', '#foo');
    });

but because it's not really an issue, I think we don't need that Cache.add change.

[vincentfretin]

I was testing with Slow 4G in Chrome, so the image took 25s to load and the timeout was 3s here.

[vincentfretin]

PR to show the loading screen before downloading the images #5779

[mrxz]

Thanks for diving into this. It seems that images will indeed be complete due to the readyState check. That said there are still two scenarios that could trigger it:

• Using the loading="lazy" attribute on images in <a-assets>
• Initializing the entire <a-scene> after the document has long loaded (e.g. loading the markup from a server and using something like containerEl.innerHTML = "<a-scene>....</a-scene>")

Arguably both are quite rare and unlikely, though people sometimes do use A-Frame in ways that the <a-scene> is only added into the document after page load (either on purpose or due to a JS framework).

Moving the Cache.add when img is complete like this [...] make the test fail because the callback materialtextureloaded is executed before the img onload, a simple setTimeout won't do, a setTimeout 10 is working but not great

I've reproduced the test case failure, and what's happening is akin to the second scenario I described above. The document is already in the "complete" readyState when the a-assets and img elements are created. But this just reveals that the test itself is flawed, the presence of the image in the cache is non-consequential to the rest of the test. Regardless when/if the image element is added to the cache, the materialtextureloaded event will take place before the image onload, meaning img.complete === false. If someone would do this in their scene, it would result in the following warning (until the image is loaded):

THREE.WebGLRenderer: Texture marked for update but image is incomplete

Despite being unlikely I do still think we should only add it to the Cache once the image is complete. Even if it wouldn't cause any harm now, it could become a very annoying bug down the line.

[vincentfretin]

Okay I did aa4db04 as part of #5778 I'll rename the title of that PR.

[mrxz]

Nitpick: we should probably avoid directly inspecting Cache.files, instead checking through THREE.Cache.get(`image:${IMG_SRC}`) instead.

[vincentfretin]

done in #5778

[mrxz]

@mrxz I believe the changes are correct but please double check.

Changes look good to me. I do believe there is a subtle bug in case an image causes the assets timeout to be hit, though.