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

Component play should not execute if the component is not initialized #1565

Merged
merged 1 commit into from Jun 29, 2016

Conversation

Projects
None yet
2 participants
@dmarcos
Copy link
Collaborator

dmarcos commented Jun 18, 2016

Description:

There's a bogus situation if play is called on a component that has not been yet initialized and the logic relies on something done in the init method.

The bug has manifested when the camera system pauses the inactive camera entity and plays the new active one. With the default camera the camera system calls play on the entity. The camera component is initialized before look-controls and its play method invoked before init. The play method of look-controls expects the methods to be bound before attaching the event listeners.

Changes proposed:
Ensure that play is never invoked before init and also prevent play/pause logic to be executed multiple times if the component is already playing or paused respectively.

@dmarcos dmarcos referenced this pull request Jun 19, 2016

Merged

Multiple components #1566

NewComponent.prototype = Object.create(Component.prototype, proto);
NewComponent.prototype.name = name;
NewComponent.prototype.constructor = NewComponent;
NewComponent.prototype.system = systems && systems.systems[name];
// Wraps method to meet needed gurantees

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

Wraps methods to meet needed guarantees.

Can also explain what the "guarantees" are.

@@ -203,6 +203,7 @@ Component.prototype = {
if (!this.initialized) {
this.init();
this.initialized = true;
if (el.isPlaying) { this.play(); }

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

Sort of confusing since seems like it should read should be playing rather than is playing. A comment might help.

This comment has been minimized.

@dmarcos

dmarcos Jun 28, 2016

Author Collaborator

Can you rephrase? What's the confusing part? What the line does it's to call the play method on the component if the corresponding entity is playing.

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

The confusing part is that isPlaying implies that it's already playing, and therefore we wouldn't need to call play().

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

Ah never mind, I got confused with entity playing vs component playing.

This comment has been minimized.

@dmarcos

dmarcos Jun 28, 2016

Author Collaborator

el referes to the entity and this to the component

This comment has been minimized.

@dmarcos

dmarcos Jun 28, 2016

Author Collaborator

for an entity that is playing we have to play its components

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

Wouldn't hurt to comment anyways.

Play the component if the entity is playing.

/**
* Pause component by removing tick behavior and calling pause handler.
*
* @param component {object} - Component to pause.

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

Incorrect parameters in docstring.

*/
function wrapPause (pauseMethod) {
return function pause () {
var sceneEl = this.el.sceneEl;

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

Could document @this is the component

function wrapPlay (playMethod) {
return function play () {
var sceneEl = this.el.sceneEl;
var canPlay = this.el.isPlaying && !this.isPlaying;

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

Probably shouldPlay

components.dummy = undefined;
var playStub = this.playStub = sinon.stub();
registerComponent('dummy', {
schema: {color: { default: 'red' }},

This comment has been minimized.

@ngokevin

ngokevin Jun 28, 2016

Member

don't really need to define a schema

@ngokevin ngokevin added needs changes and removed needs review labels Jun 28, 2016

@dmarcos dmarcos force-pushed the dmarcos:componentPlay branch from ff3ce93 to 1892448 Jun 28, 2016

@dmarcos dmarcos force-pushed the dmarcos:componentPlay branch from 1892448 to 1383c4a Jun 28, 2016

@dmarcos dmarcos added needs review and removed needs changes labels Jun 28, 2016

@dmarcos dmarcos merged commit fee93fc into aframevr:master Jun 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment