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

fix dynamically-attached entities not playing due to load conditions (fixes #1383) #1415

Merged
merged 1 commit into from
May 13, 2016

Conversation

ngokevin
Copy link
Member

@ngokevin ngokevin commented Apr 27, 2016

Description:

Caught this using the template component that dynamically attached entities that also had the template component to dynamically attach entities.

  1. Entity is attached.
  2. Entity tries to play()
  3. Entity not yet loaded
  4. Entity never plays because not loaded (early return statement)

Changes proposed:

  • Animations wait for entity to both load + play before playing.
  • Ensure entities call play() after they are loaded.
  • Remove unnecessary .bind calls.

@ngokevin ngokevin changed the title [needs tests] fix dynamically-attached entities not playing due to load conditions (fixes #1383) fix dynamically-attached entities not playing due to load conditions (fixes #1383) Apr 29, 2016
}
} else {
// To handle elements that are not yet `<a-entity>`s (e.g., templates).
el.addEventListener('nodeready', init.bind(self));
Copy link
Member

Choose a reason for hiding this comment

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

we don't listen to the nodeready event anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those were leftover from <a-template>.

@ngokevin
Copy link
Member Author

@dmarcos r?

}

// Entity load.
function entityLoadCallback () {
Copy link
Member

@dmarcos dmarcos May 12, 2016

Choose a reason for hiding this comment

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

Can we move this function to the bottom of the function after ANode.prototype.load.call(this, entityLoadCallback, isEntity);?

Copy link
Member Author

Choose a reason for hiding this comment

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

any reason to? seems more consistent with doing vars-on-top since these functions get hoisted. and readability-wise, it makes sense to want to know what this function is doing in the context of the method.

Copy link
Member

@dmarcos dmarcos May 13, 2016

Choose a reason for hiding this comment

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

The function gets in the way of understanding the main body of the function. I want to see what the function does first and maybe later want to see what happens asynchronously when the entity loads.

Copy link
Member

Choose a reason for hiding this comment

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

I'll change this later in a follow up PR

Copy link
Member Author

Choose a reason for hiding this comment

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

the function is integral to understanding what the function does though. it's only separated because it's awkward to inline

@dmarcos dmarcos merged commit 793985e into aframevr:master May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants