Skip to content

Fix handling image/file cache for three r178 and above #5776

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

Merged
dmarcos merged 2 commits into from
Dec 15, 2025
+14 −17
Merged

Fix handling image/file cache for three r178 and above #5776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3451163
Fix handling image/file cache now that the ImageLoader/FileLoader are…
vincentfretin Dec 14, 2025
ff3355a
Don't set cache for video, it's not used
vincentfretin Dec 14, 2025
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
9 changes: 1 addition & 8 deletions src/core/a-assets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ 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(imgEls[i].getAttribute('src'), imgEl);
THREE.Cache.add('image:' + imgEls[i].getAttribute('src'), imgEl);
Copy link
Contributor

@mrxz mrxz Dec 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vincentfretin vincentfretin Dec 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vincentfretin vincentfretin Dec 16, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@vincentfretin vincentfretin Dec 16, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mrxz mrxz Dec 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@vincentfretin vincentfretin Dec 17, 2025

Choose a reason for hiding this comment

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

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

if (imgEl.complete) {
resolve();
return;
Expand Down Expand Up @@ -166,13 +166,6 @@ function mediaElementLoaded (el) {

// Compare seconds buffered to media duration.
if (secondsBuffered >= el.duration) {
// Set in cache because we won't be needing to call three.js loader if we have.
// a loaded media element.
// Store video elements only. three.js loader is used for audio elements.
// See assetParse too.
if (el.tagName === 'VIDEO') {
THREE.Cache.add(el.getAttribute('src'), el);
}
resolve();
}
}
Expand Down
10 changes: 7 additions & 3 deletions tests/components/material.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,16 @@ suite('material', function () {
var textureLoaderSpy = this.sinon.spy(THREE.TextureLoader.prototype, 'load');
img.setAttribute('src', IMG_SRC);
img.setAttribute('id', 'foo');
THREE.Cache.files[IMG_SRC] = img;
Copy link
Contributor Author

@vincentfretin vincentfretin Dec 14, 2025

Choose a reason for hiding this comment

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

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.

THREE.Cache.clear();
assetsEl.appendChild(img);
el.sceneEl.appendChild(assetsEl);
// Adding the asset will add image:${IMG_SRC} in THREE.Cache.files
// without going through THREE.ImageLoader
el.addEventListener('materialtextureloaded', function () {
assert.notOk(imageLoaderSpy.called);
assert.notOk(textureLoaderSpy.called);
delete THREE.Cache.files[IMG_SRC];
assert.ok(`image:${IMG_SRC}` in THREE.Cache.files);
Copy link
Contributor

@mrxz mrxz Dec 15, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@vincentfretin vincentfretin Dec 16, 2025

Choose a reason for hiding this comment

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

done in #5778

THREE.Cache.clear();
THREE.ImageLoader.prototype.load.restore();
THREE.TextureLoader.prototype.load.restore();
done();
Expand All @@ -191,10 +194,11 @@ suite('material', function () {
var imageLoaderSpy = this.sinon.spy(THREE.ImageLoader.prototype, 'load');
el.addEventListener('materialtextureloaded', function () {
assert.ok(imageLoaderSpy.called);
assert.ok(IMG_SRC in THREE.Cache.files);
assert.ok(`image:${IMG_SRC}` in THREE.Cache.files);
THREE.ImageLoader.prototype.load.restore();
done();
});
// The image is loaded via the material system using THREE.ImageLoader
el.setAttribute('material', 'src', IMG_SRC);
});

Expand Down
12 changes: 6 additions & 6 deletions tests/core/a-assets.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ suite('a-assets', function () {
assetsEl.appendChild(img);

img.addEventListener('load', function () {
assert.equal(THREE.Cache.files[IMG_SRC], img);
assert.equal(THREE.Cache.files[`image:${IMG_SRC}`], img);
done();
});

Expand Down Expand Up @@ -248,7 +248,7 @@ suite('a-asset-item', function () {
});

test('emits progress event', function (done) {
THREE.Cache.remove(XHR_SRC);
THREE.Cache.remove(`file:${XHR_SRC}`);
var assetItem = document.createElement('a-asset-item');
assetItem.setAttribute('src', XHR_SRC);
assetItem.addEventListener('progress', function (evt) {
Expand Down Expand Up @@ -284,7 +284,7 @@ suite('a-asset-item', function () {
assetItem2.setAttribute('src', XHR_SRC);

// Remove cache data to not load from it.
THREE.Cache.remove(XHR_SRC);
THREE.Cache.remove(`file:${XHR_SRC}`);

assetItem1.addEventListener('error', function (evt) {
assert.ok(evt.detail.xhr !== undefined);
Expand All @@ -306,7 +306,7 @@ suite('a-asset-item', function () {
test('loads as text without responseType attribute', function (done) {
var assetItem = document.createElement('a-asset-item');
// Remove cache data to not load from it.
THREE.Cache.remove(XHR_SRC);
THREE.Cache.remove(`file:${XHR_SRC}`);
assetItem.setAttribute('src', XHR_SRC);
assetItem.addEventListener('loaded', function (evt) {
assert.ok(assetItem.data !== null);
Expand All @@ -319,7 +319,7 @@ suite('a-asset-item', function () {

test('loads as arraybuffer', function (done) {
var assetItem = document.createElement('a-asset-item');
THREE.Cache.remove(XHR_SRC);
THREE.Cache.remove(`file:${XHR_SRC}`);
assetItem.setAttribute('src', XHR_SRC);
assetItem.setAttribute('response-type', 'arraybuffer');
assetItem.addEventListener('loaded', function (evt) {
Expand All @@ -344,7 +344,7 @@ suite('a-asset-item', function () {
});

test('reloads as text', function (done) {
THREE.Cache.remove(XHR_SRC);
THREE.Cache.remove(`file:${XHR_SRC}`);
var assetItem = document.createElement('a-asset-item');
assetItem.setAttribute('src', XHR_SRC);
assetItem.addEventListener('loaded', function (evt) {
Expand Down