Possible fix for #5032: Don't hang forever on asset loading error

[diarmidmackenzie]

Description:

Currently, if a bad URL is specified for an asset, A-Frame just shows the loading screen indefinitely (regardless of the asset timeout configured).

This prevents applications from providing application-specific handling (e.g. their own error message) when this might happen. (in terms of why this might happen, suppose the HTML content is generated by server-side rendering on a back-end, and a bug emerges in the backend).

Changes proposed:

Update a-node processing when awaiting all children loading, so that:

• it acknowledges the possibility of errors, and handles them
• goes ahead and declares items "loaded" so that the scene can render, even if there have been errors.

I also added a small example showing the problem: /examples/test/gltf-model-error/

Not sure if you'll want this included as part of the PR, but it was handy for reproducing the problem & validating the fix.

[diarmidmackenzie]

Sorry for the slow progress on this.

I've now added some test cases.

It took me a long time to realize that Mocha is very sensitive to detecting unhandled errors, and will fail tests whenever your code emits an event named "error", and your test script does not immediately call done(), or stop event propagation.

The error message for this is rather opaque...

Error: [object CustomEvent] (undefined:undefined)

I have also adjusted the fix itself. Previously, as soon as a single faulty asset was found, it would jump into the scene. Now it proceeds with the rest of asset loading on the normal pattern, and logs warnings about the failed assets at the end.

I think that's better behaviour: as per the original issue, we want to mitigate the impact of one incorrect asset URL, but that doesn't mean we want to jump into rendering the scene before other, available, assets have had a chance to load.

[diarmidmackenzie]

I've left the gltf_model_error.html test file in for now, as I found it pretty useful while working on this, so it could be useful in future?

But happy to remove it if you prefer...

[diarmidmackenzie]

@dmarcos, with the UTs done, can this be merged now? Or are there other concerns to address?

@kylebakerio has reached out to ask whether I can push ahead and get this merged.

[vincentfretin]

@dmarcos you should probably merge this first, then I will rebase #5123 to resolve a conflict.

[vincentfretin]

@diarmidmackenzie can you please rebase your PR, you need to fix the conflict around evt.stopPropagation(); since #5123 was merged first.

[diarmidmackenzie]

@diarmidmackenzie can you please rebase your PR, you need to fix the conflict around evt.stopPropagation(); since #5123 was merged first.

Done.

[dmarcos]

This needs a rebase. Combing through open stuff to prep for a 1.4.0 release. Thanks for the patience

[diarmidmackenzie]

I believe this is now all up-to-date with latest master again.

A couple of conversation points where I'm not 100% sure whether you were happy with what I'd proposed or not.. https://github.com/aframevr/aframe/pull/5033#discussion_r984745019 https://github.com/aframevr/aframe/pull/5033#discussion_r833226414

[diarmidmackenzie]

Hmm - seing a test failure like this: SUMMARY: ✔ 734 tests completed ℹ 3 tests skipped ✖ 1 test failed

FAILED TESTS: ✖ "after each" hook for "does not try to enter VR if already in VR"

I'll look into it.

[diarmidmackenzie]

OK, I don't know why this test issue has only come to light now, but it is due to inadequate mocking of the canvas element.

This leads to an exception in the look-controls component when it tries to add to the canvas classList.

TypeError: Cannot read properties of undefined (reading 'add')
      at enableGrabCursor (build/commons.js:17060:32)
      at NewComponent.updateGrabCursor (build/commons.js:17074:7)
      at NewComponent.update (build/commons.js:16723:12)
      at NewComponent.initComponent (build/commons.js:26161:10)
      at NewComponent.updateProperties (build/commons.js:26130:12)
      at AEntity.updateComponent (build/commons.js:24971:17)
      at AEntity.updateComponents (build/commons.js:24948:12)
      at entityLoadCallback (build/commons.js:24739:12)
      at emitLoaded (build/commons.js:25673:9)

I believe the solution (which I've just added as a fix into this PR) is to extend the mock canvas object so that classList.add and classList.remove can be invoked without causing exceptions.

I don't have a good understanding of why this didn't cause problems prior to this fix, but the exception does occur in a loading callback, and we have made changes in the handling of such exceptions, so it's plausible that for some reason this exception didn't cause problems for the tests previously.

This particular failng test is not intended to test exceptions, and the intent of the test will be better met if we avoid these exceptions, which is what the extension to the canvas mock object achieves. So I think that is the correct fix, and I don't see any evidence of a problem in the code under test.

[kylebakerio]

Vincent has made many changes asnd improvements to the tests in recent months, part of which allowed many tests that were being skipped to now be run. Could be related to that, I didn't look into those changes in too much detail.

Good catch and fix.

[vincentfretin]

There is also an error on master with firefox related to classList that is not mocked properly https://github.com/aframevr/aframe/actions/runs/3520429062/jobs/5901336232 I don't know either why it shows up only now. It may be good to create a PR just for this mocking fix if this PR is not merged right away.

[vincentfretin]

I created #5176 with a slightly modified version of the fix @diarmidmackenzie

[diarmidmackenzie]

@dmarcos Do you want to take this fix? Or give more feedback on the discussion items above?

[dmarcos]

We can merge this once we remove the example. Thanks for sticking with it.

[diarmidmackenzie]

Ok - removed that file from examples\test so I think we're good now.

[dmarcos]

Thanks so much. Super appreciated