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

Schema may not be registered when the networked component is initialized #267

Open
vincentfretin opened this issue Mar 27, 2021 · 15 comments
Labels

Comments

@vincentfretin
Copy link
Member

vincentfretin commented Mar 27, 2021

There can be a race condition case where the schema defined in the footer of the page is not register when the networked init function is executed where it calls NAF.schemas.getComponents to get the components to sync based on the registered template

this.componentSchemas = NAF.schemas.getComponents(this.data.template);

The issue seems to be always reproducible when there is no assets to download.
I created an example based on the naf-project template where I only commented the two assets sky and grid, and modified the template to not use random-color, which hide the issue actually...

https://glitch.com/edit/#!/naf-schema-race-condition

The issue here is that all avatars stay red, instead of a random color set by the participant and normally synced to other participants.
In the logs we get this order:

  • calling NAF.schemas.getComponents from networked component init
  • components in schema: ["position", "rotation"]
  • register schema
  • scene loaded

With this order, NAF.schemas.getComponents returns the default ["position", "rotation"] instead of ["position", "rotation", {selector: ".head", component: "material", property: "color"}], so not syncing the material color set by the random-color component on the player entity.

A workaround is to not set the networked component in the html, but set it with javascript:

     <script>
      document.addEventListener('DOMContentLoaded', () => {                           
        const scene = document.querySelector('a-scene');
        const sceneLoaded = () => {
          console.log("scene loaded");
          // Edited after writing the next comment (you need to register the schema just before using setAttribute)
          NAF.schemas.add({
            template: '#avatar-template',
             components: [
              'position',
              'rotation',
              {
                selector: '.head',
                component: 'material',
                property: 'color'
              }
            ]
          });
          document.getElementById("player").setAttribute("networked", "template:#avatar-template;attachTemplateToLocal:false;");
        }
        if (scene.hasLoaded) {                                                         
          sceneLoaded();
        } else {
          scene.addEventListener('loaded', sceneLoaded);
        }
      });
    </script>

and remove networked="template:#avatar-template;attachTemplateToLocal:false;" from <a-entity id="player"

I'm not sure how we can fix that... or if we can fix that really...

The issue may appear randomly when you have small asset in <a-assets> to download or if you have it already in your cache.
The issue is much worse when you register a template that doesn't have position and rotation, you can end up with two participants having a different schema, one sending a vec3 trying to fit as a number in a custom component and you have weird things happening like objects disappearing. Yes I had this case on one of my projects. :D

@vincentfretin
Copy link
Member Author

vincentfretin commented Mar 27, 2021

Actually the workaround I'm describing here may not be enough. I think in my app I had cases but I'm not sure anymore were "register schema" was after "scene loaded". So just to be safe, you can use networked-scene="connectOnLoad:false" and a button onClick that calls:

AFRAME.scenes[0].emit('connect');
document.getElementById("player").setAttribute("networked", "template:#avatar-template;attachTemplateToLocal:false;");

or better in the previous workaround described in the previous comment, call NAF.schemas.add just before the line

document.getElementById("player").setAttribute("networked", "template:#avatar-template;attachTemplateToLocal:false;");

so you are sure of the order.

@vincentfretin
Copy link
Member Author

vincentfretin commented Mar 27, 2021

In my app where I had the issue randomly before when using networked component in the html, I removed all networked component from the html and I'm using a single js file with something like this:

AFRAME.registerComponent('slideshow', {
    schema: {
      index: {type: 'number', default: 0},
      src: {type: 'string', default: ""},
    },
    init: function () {
      const addNetworkedComponent = () => {
        this.el.setAttribute('networked',
          { template: '#slideshow-template',
            attachTemplateToLocal: false,
            networkId: this.el.id,
            persistent: true,
            owner: 'scene'
          }
        );
         NAF.utils
          .getNetworkedEntity(this.el)
          .then(networkedEl => {
            this.networkedEl = networkedEl;
          })
          .catch(() => {
            // Non-networked
          });
      };
      // In some cases, NAF.schemas may not be populated yet
      // (this init is executed before the DOMContentLoaded listener where we call NAF.schemas.add to register the schema),
      // so adding networked component in init may result in the component
      // using wrong default schema position/rotation, so we wait for scene loaded that comes after DOMContentLoaded event
      // (This comment is absolutely false, scene loaded can be before DOMContentLoaded and we go into the
      // this.el.sceneEl.hasLoaded branch here, all this code is not enough, see last comment with updated code)
      if (this.el.sceneEl.hasLoaded) {
        addNetworkedComponent();
      } else {
        this.el.sceneEl.addEventListener('loaded', addNetworkedComponent);
      }
    }
    ...
}

document.addEventListener('DOMContentLoaded', function() {
  NAF.schemas.add({template: '#slideshow-template', components ["slideshow"]})
})

and in my html

<a-plane id="main-screen" slideshow></a-plane>

So here scene loaded always come after DOMContentLoaded so this is good. (Edit: this assumption is false, see latest comment)

So maybe the real fix to do in networked-aframe is to wait for scene loaded event before calling NAF.schemas.getComponents in networked component init function.

@leo-nerd
Copy link

leo-nerd commented Apr 7, 2021

I am experiencing the same bug. Thanks @vincentfretin for describing your tests, I'll look into them.
For what it's worth: this is happening to me only when the app is deployed to a remote machine (e.g. Heroku) but cannot be reproduced locally.

@vincentfretin
Copy link
Member Author

Ahhhhh, with my previous slideshow code that worked great since june last year, I just had an error "this.data.src.startsWith is not a function" with someone on Safari that I suspect sent to me bad data probably components: [{x: 0, y: 0; z: 0}, rotation: {x: 0, y: 0, z: 0}] instead of components: [0, "someurl"] I'm waiting. The error startsWith is not a function is triggered when src is an object. :(
So I think the Safari user hit the this.el.sceneEl.hasLoaded branch and the schema wasn't registered at the time the networked element was created, this is similar than declaring networked in the html. I have now the error because I have now zero images in a-assets since some weeks.

@vincentfretin
Copy link
Member Author

getComponents(template) {
var components = ['position', 'rotation'];
if (this.hasTemplate(template)) {
components = this.schemaDict[template].components;
}
return components;
}

should log at least an error if the template is not found when template is not '' which is the default
template: {default: ''},

Ideally getComponents(template) should be delayed if the template doesn't currently exist, wait 1s, check, wait 1s, check, and give an error after 3s? In this case it should return a Promise and we need to adapt the code because this.componentSchemas will be set asynchronously. This is not a small change.

I'll add a setTimeout 3s before calling addNetworkedComponent() in my this.el.sceneEl.hasLoaded branch lol or create some function ensureTemplateIsRegistered before calling setAttribute('networked', ...), may be better. :)

@vincentfretin
Copy link
Member Author

vincentfretin commented Apr 12, 2021

Ok I will use this now in my code. I added a registerSchema function were I check if template is already registered or not with NAF.schemas.hasTemplate, register it if not registered. And I'm calling that before setAttribute('networked', ...)

AFRAME.registerComponent('slideshow', {
    schema: {
      index: {type: 'number', default: 0},
      src: {type: 'string', default: ""},
    },
    init: function () {
      const addNetworkedComponent = () => {
        registerSchema();  // This was added
        this.el.setAttribute('networked',
          { template: '#slideshow-template',
            attachTemplateToLocal: false,
            networkId: this.el.id,
            persistent: true,
            owner: 'scene'
          }
        );
         NAF.utils
          .getNetworkedEntity(this.el)
          .then(networkedEl => {
            this.networkedEl = networkedEl;
          })
          .catch(() => {
            // Non-networked
          });
      };
      // wait for scene loaded to be sure the template exists in a-assets
      if (this.el.sceneEl.hasLoaded) {
        addNetworkedComponent();
      } else {
        this.el.sceneEl.addEventListener('loaded', addNetworkedComponent);
      }
    }
    ...
}
function registerSchema() {
  if (!NAF.schemas.hasTemplate('#slideshow-template')) {
    NAF.schemas.add({template: '#slideshow-template', components ["slideshow"]})
  }
}
// document.addEventListener('DOMContentLoaded', function() {
//  NAF.schemas.add({template: '#slideshow-template', components ["slideshow"]})
// })

I'm keeping the scene loaded logic to be sure that the template is loaded from the DOM, I had a case on a simple glitch example where document.querySelector in component init didn't find the node (which was outside of <a-assets>, in body before <a-scene>), maybe it's not an issue with <template> defined in <a-assets>, but I prefer to keep that.
registerSchema is calling NAF.schemas.add that calls document.querySelector('#slideshow-template')

@vincentfretin
Copy link
Member Author

I edited my comment with the non working code. My assumption was wrong, scene loaded can be before DOMContentLoaded and we go into the this.el.sceneEl.hasLoaded branch here.

@devinbean1
Copy link

devinbean1 commented Apr 13, 2021

Hi Vincent,

We were able to fix this by loading an image in the following tag:
<assets><img src="https://wallpapercave.com/wp/wp2345390.jpg?dummy=8484744"></assets>

We had to include the dummy query ?dummy=8484744 though because it would only work on first load and then the image would get cached. The dummy query ensures the image gets loaded every time. This seems like a workaround though and that the actual bug is something timing related in NAF. Do you know what is causing this under the hood?

-Devin

@vincentfretin
Copy link
Member Author

Hi, putting a query string in the url doesn't cache it? interesting.
Yes I know very well what is the issue, the whole thread explains it. It's a race condition between the registration of the schema and the networked init execution. I worked around the issue by not using networked in the html, but doing it in javascript and be sure the schema is registered before using a networked component.
I commented on how we could fix the issue in the networked-aframe library, but it's not a trivial change.

@devinbean1
Copy link

Thank you. I believe we implemented your fix correctly but now getting issue where some people load and are stuck in the middle of the room. I remember seeing this before with the basic NAF examples last summer. Is this related to this bug or is this something different?

@vincentfretin
Copy link
Member Author

What do you mean by stuck? Please create another issue for this to not pollute this issue, thank you. Or you can join me on slack so we can talk about it.

@vincentfretin
Copy link
Member Author

Another workaround that can be simpler so that you can still use networked component in the html is to patch getComponents like this in the <head>:

 <script>
      NAF.schemas.getComponentsOriginal = NAF.schemas.getComponents;
      NAF.schemas.getComponents = (template) => {
        if (!NAF.schemas.hasTemplate("#avatar-template")) {
          NAF.schemas.add({
            template: '#avatar-template',
             components: [
              'position',
              'rotation',
              {
                selector: '.head',
                component: 'material',
                property: 'color'
              }
            ]
          });
        }
        const components = NAF.schemas.getComponentsOriginal(template);
        return components;
      }
</script>

It's a little bit hacky but this works great. This will do an extra if when initializing each networked component, but the hasTemplate function is just checking if the key exist in an object, so it shouldn't matter much in term of performance.

@vincentfretin
Copy link
Member Author

I modified https://glitch.com/edit/#!/naf-project with the above workaround.

@arpu
Copy link
Member

arpu commented Mar 28, 2022

not sure if this helps what i do is to wait for adapter-ready and than register the schemas

sceneEl.addEventListener( "adapter-ready", async ( { detail: adapter } ) => {
registerNetworkSchemas();

}

@vincentfretin
Copy link
Member Author

Yep that works too if you're using connectOnLoad:false. But this may still trigger the issue with connectOnLoad:true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants