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

Apply stashed first sync on videos before playback and ownership #1556

Merged
merged 3 commits into from
Jul 24, 2019

Conversation

brianpeiris
Copy link
Contributor

Fixes #1543 by having networked-aframe stash first-syncs for persistent entities, instead of creating said entities. We then apply the stashed first-sync before deciding on playback state and ownership.

Goes with:

@brianpeiris brianpeiris requested a review from gfodor July 23, 2019 21:41
@@ -361,6 +361,7 @@ AFRAME.registerComponent("media-video", {

NAF.utils.getNetworkedEntity(this.el).then(networkedEl => {
this.networkedEl = networkedEl;
this.networkedEl.components.networked.applyPersistentFirstSync();
Copy link
Contributor

Choose a reason for hiding this comment

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

as noted in the other PR, any reason we can't push this policy into NAF itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't apply the stashed sync in init on networked, the rest of the entity's components are not initialized at that point. If we did apply it then, it would result in those components being created and then overwritten by the rest of Hub's scene and media initialization process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular, we have to apply the first sync after component inits, but before we do this very next bit, where we try to take ownership of scene-owned media.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah interesting. i guess i can't come up with a better idea. renaming the function to be less implementation-centric would be nice, since it's a public API, but i can't think of a much better name. eg notifyPeristentEntityReady or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, actually I think I would like to keep the name explicit since I want it to be clear to us when reading this code.

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.

Scene owned videos have a race condition
2 participants