Skip to content
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.

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

Allow async loading of aframe library and interoperation with ES module libraries #5419

Closed
wants to merge 2 commits into from

Conversation

chrisirhc
Copy link
Contributor

@chrisirhc chrisirhc commented Dec 21, 2023

Description:
This allows interop with ES module libraries such as webxr.
ES modules are a part of the Web Component spec but it's a bit surprising that aframe doesn't play well with other Web Components that rely on ES modules.

It allows aframe to be loaded as a module or deferred on page load.
Currently, aframe needs to be loaded synchronously. Adding async or defer or type="module" can cause aframe library to fail in many different ways such as:

  • a-scene is registered and connectedCallback hook does not find the camera component as it hasn't been registered
  • a-entity's connectedCallback hook unable to find that a-scene is initialized

These proposed changes allow the following code snippet to run:
Note that new LookingGlassWebXRPolyfill() needs to run before importing aframe in order to polyfill/override webxr for looking glass capabilities.

    <script type="module">
      import { LookingGlassWebXRPolyfill } from "@lookingglass/webxr"
      new LookingGlassWebXRPolyfill()
      window.AFRAME_ASYNC = true;
      await import("aframe")
      // add any components
      await import("https://quadjr.github.io/aframe-gaussian-splatting/index.js")
      window.AFRAME.ready();
    </script>

Example of this working is at https://chrisirhc.github.io/looking-glass-viewer-experiments/aframe.html .

Changes proposed:

  • Delay running doConnectedCallback until ready is called if window.AFRAME_ASYNC is set to a truthy value
  • Reorder the initialization in index.js so that default components and system are registered before connectedCallback is called.

This is a strawman implementation. I'm not sure how to name these API surfaces and open to suggestions.

Alternatives considered

  • Change the order or delay order in which the customElements are registered.
    • Not that this doesn't work well because a-scene element creates a-entity elements during its connectedCallback lifecycle hook, so if a-entity isn't already registered, then

@@ -11,6 +11,17 @@ var MULTIPLE_COMPONENT_DELIMITER = '__';
var OBJECT3D_COMPONENTS = ['position', 'rotation', 'scale', 'visible'];
var ONCE = {once: true};

var _resolveReadyPromise;
Copy link
Member

Choose a reason for hiding this comment

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

Can name just resolveReadyPromise. We don't use ampersand

@dmarcos
Copy link
Member

dmarcos commented Dec 21, 2023

Thanks so much!

@vincentfretin @diarmidmackenzie what do you think? Sorry for the ping. Have the feeling you know more about this than me :)

At first glance it feels weird tying the logic to the Entity load when async is supposed to be at library level. But maybe. Not sure yet.

src/core/scene/a-scene.js Outdated Show resolved Hide resolved
@vincentfretin
Copy link
Contributor

The changes looks ok. I don't really have better.
Do we need a similar fix in src/core/scene/a-scene.js to delay executing of this.initSystems() if you imported a module that registered a system?

I guess this PR fixes this old issue #4038

@dmarcos
Copy link
Member

dmarcos commented Dec 21, 2023

Just curious how a complete example would look like?

await import("aframe") I imagine 'aframe' has to defined somehow previously.

I'm still getting familiar with ES6 modules.

Would be nice to see one full basic example: https://glitch.com/~aframe

@vincentfretin
Copy link
Contributor

You need to define the importmap. In his linked example, this is defined:

    <script type="importmap">
      {
          "imports": {
              "@lookingglass/webxr": "./lib/webxr.module.js",
              "aframe": "./lib/aframe-master.js"
          }
      }
    </script>

You can replace the path by a jsdelivr url.

@dmarcos
Copy link
Member

dmarcos commented Dec 22, 2023

Wouldn't be better to tie the promise to A-Frame loading as a whole vs when the first entity connects? For example here: https://github.com/aframevr/aframe/blob/master/src/index.js#L95

@chrisirhc
Copy link
Contributor Author

chrisirhc commented Dec 22, 2023

Just curious how a complete example would look like?

await import("aframe") I imagine 'aframe' has to defined somehow previously.

You could use the importmaps or, more typically, It'd look like the full JS url:

await import("https://aframe.io/releases/1.5.0/aframe.min.js")

The import() and await statements are to ensure the side effects of the modules are run in the order in the code. See
reference .

Wouldn't be better to tie the promise to A-Frame loading as a whole vs when the first entity connects? For example here: https://github.com/aframevr/aframe/blob/master/src/index.js#L95

Ah, the change I'm proposing is to run the entity connectedCallback after A-Frame loading has completed.
The setTimeout achieves this since it waits until all the synchronous loading/execution has completed.

That said, the problem it works around is that the connectedCallback expect that the A-Frame loaded has completed, but these connectedCallback are being run during registerElement because the DOM is already available during the async loading.

Typically, the steps look like:

  1. Load script synchronously via <script src=https://aframe.io/releases/1.5.0/aframe.min.js></script> in the head. This includes running customElement.register but just registers the element and doesn't call the callbacks.
  2. Parse the DOM including the body that contains <a-scene> and other elements. During the parse, constructor and callbacks (connectedCallback) are called for each element as they're encountered.

However, in the async/module flow, the steps look like:

  1. Parse the DOM, including the body that contains <a-scene> and other elements. Does not call custom element constuctors because the module hasn't loaded yet.
  2. Then while loading + executing the aframe script:
  3. As it encounters the line customElement.register('a-scene', …) code, it runs the constructors and connectedCallback.
  4. Continues to execute the rest of the aframe code.

The problem is that in Step 3, not all of the aframe code has executed. So things like the camera weren't registered. This is only a problem because the connectedCallback was called during parsing/execution of the aframe script.
It's not a problem if all of the aframe script has already executed.

Hope this provides an explanation of what's going on.
This is what I gather was happening as I was working through this issue.

@mrxz
Copy link
Contributor

mrxz commented Dec 22, 2023

There is already logic in place to defer the initialization till the document.readyState is complete. I feel it would be cleaner to generalize this logic instead of adding additional flows. Instead of listening for the readystatechange it could listen to a dedicated event, which by default would still be based on the ready state.

As for the magic AFRAME_ASYNC variable, I don't really like it as it's easily something someone can forget or get wrong. Perhaps it's possible for A-Frame to detect it's being loaded asynchronously and automatically switch to expecting a ready signal from the user?
Ideally there would then be a warning logged to the console if this signal isn't given, but not sure if there is a logical moment to detect this apart from a timeout. Likewise an error if the ready signal is given more than once, or if components/systems are registered after the signal has been given.

Example of this working is at https://chrisirhc.github.io/looking-glass-viewer-experiments/aframe.html .

This example doesn't work in Firefox for me, but does on Chrome. Didn't dive into the cause, but it seems there is some difference in execution order between the two.

As an aside, the way I generally workaround the issue is by placing the scene in a template and adding it to the DOM after loading and registering all dependencies:

<html>
    <head>
        <script type="importmap">
            (...)
        </script>
        <script type="module">
            await import("aframe");

            // Load dependencies

            const sceneTemplate = document.getElementById('scene');
            document.body.appendChild(sceneTemplate.content)
        </script>
    </head>
    <body>
        <template id="scene">
            <a-scene>
                <a-box id="box" color="red" position="0 1.6 -2"></a-box>
            </a-scene>
        </template>
    </body>
</html>

Comment on lines +14 to +23
var _resolveReadyPromise;
var isReady = false;
var isReadyPromise = new Promise(function (resolve) {
_resolveReadyPromise = resolve;
});

function ready () {
isReady = true;
_resolveReadyPromise();
}
Copy link
Contributor Author

@chrisirhc chrisirhc Dec 22, 2023

Choose a reason for hiding this comment

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

Looking at this again, it might make more sense for this "ready" logic to live in its own file/module that just governs whether the entire aframe library is ready. I can tidy this up.

This initial PR was only to gather and understand if there's interest/appetite for such a change.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good. thanks. In an ideal world we would have a single build that can be loaded via modules (export, import) or script tag. As far as I know that's not possible, right?

@dmarcos
Copy link
Member

dmarcos commented Dec 22, 2023

As an aside, the way I generally workaround the issue is by placing the scene in a template and adding it to the DOM after loading and registering all dependencies:

Nice workaround. Hopefully we can find a way for A-Frame to work seamlessly with both modules and script tags. I would like to also avoid two separate builds (aframe.js, aframe.module.js) but not sure if possible. Biggest downside of ES6 modules is that how it seems to split JS space in two: pre and post modules. Also I think there's still valuable simplicity on loading JS with just a <script> tag and no additional incantations.

@arpu
Copy link
Contributor

arpu commented Dec 22, 2023

not sure where the problem is at the moment what still works is
https://glitch.com/edit/#!/ink-zealous-pudding

@arpu
Copy link
Contributor

arpu commented Dec 22, 2023

i think a good example is model-viewer https://modelviewer.dev/

@dmarcos
Copy link
Member

dmarcos commented Dec 27, 2023

Let me know if you need help or any questions to move this along. Thanks!

@vincentfretin
Copy link
Contributor

@arpu In your glitch example you don't async load any new component, so of course you don't have any issue.
I didn't understand the reference to modelviewer, this is a module js yes, just a single web component. For aframe the issue is component on a web component, the component is not yet loaded when the web component is initialized.

@chrisirhc on my comment #5419 (comment) need to test to async load a system.

@arpu
Copy link
Contributor

arpu commented Dec 28, 2023

Thanks @vincentfretin you are right not trying to load second componet, and now i see the problem
for the modelviewer refrence was more for @dmarcos to see how easy it is today with es module and script tag

@chrisirhc
Copy link
Contributor Author

@chrisirhc on my comment #5419 (comment) need to test to async load a system.

Hi, do you mind sharing an example on how I can do this?

@vincentfretin
Copy link
Contributor

You can try loading the uvscroll.js as a module, that includes a uv-scroll system and uv-scroll component, see c-frame/sponsorship#15

@dmarcos
Copy link
Member

dmarcos commented Jan 13, 2024

There are some pending changes here

@vincentfretin
Copy link
Contributor

I created a glitch with the same example as @chrisirhc and the uv-scroll system/component, it's working properly as far as I can tell.
https://glitch.com/edit/#!/vfretin-async-aframe

@dmarcos
Copy link
Member

dmarcos commented Feb 1, 2024

@chrisirhc There are few pending changes from the review (e.g underscore on name variables). Do you want to finish it up? Thanks so much!

@chrisirhc
Copy link
Contributor Author

Hi there, apologies for the radio silence.
I likely won't be able to get to this any time soon.

I was stuck on a blurness issue ( mkkellogg/GaussianSplats3D#65 (comment) ) after finding out that aframe using a pretty old/forked version of ThreeJS so I've had to look elsewhere despite hacking this fix together.

I might not get back to this in the near future. Feel free to close this out.

@dmarcos
Copy link
Member

dmarcos commented Mar 27, 2024

@chrisirhc FYI A-Frame master is on latest THREE r162. Can use builds linked in commits https://github.com/aframevr/aframe/commits/master/

@vincentfretin
Copy link
Contributor

Also you can close this PR now that #5481 is merged.

@chrisirhc
Copy link
Contributor Author

chrisirhc commented Mar 27, 2024

Got it! Thanks @mrxz for getting the change to the finish line!
I'll be able to test things out with the main branch now, with the above suggestions. 👍

@chrisirhc chrisirhc closed this Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants