Store oldData after building component data. undefined passed to upda… #2840

dmarcos · 2017-07-04T07:06:00Z

…te on component initialization (fix #2829)

The following call to getAttribute returns the data object of the component (this happens due to one of the perf optimization to reduce unnecessary object copies) that gets modified and then passed to the setAttribute method.

var position = el.getAttribute('position');
position += 1;
el.setAttribute('position', position);

the oldData object was constructed before building the data object resulting in the oldData to actually contain the current data. The condition that checks if the component should be updated (oldData !== data) was failing because the objects were identical.

Two fundamental changes in this PR:

The oldData object is built after the data object is constructed to contain the actual previous data.
The update method receives undefined on first call when the component initializes. The core components have been modified to comply.

ngokevin · 2017-07-05T22:49:48Z

Can we avoid making oldData undefined on the first call? It's extra work to check and opens up for potential for lots of bugs in components (e.g., not accounting it could be both undefined or an object). Can we separate the internal old data versus the public one?

dmarcos · 2017-07-05T22:57:32Z

Old Data being undefined it what makes sense to me? What do you pass to update on first call, when the component has been just initialized?

ngokevin · 2017-07-05T23:00:17Z

Empty data makes just as much sense. And it's backwards incompatible and puts less type checking effort and less potential bugs for component authors.

What do we return where?

dmarcos · 2017-07-05T23:01:27Z

What about components that don't have an object as schema data type like visible for instance? If the default is false people might do things like if (oldData) {...}

ngokevin · 2017-07-05T23:54:11Z

undefined works better for single-prop schemas. I think that was the current behavior.

dmarcos · 2017-07-06T00:07:04Z

Ok, I'll revert back to passing {} on multi prop schemas and undefined on single property

ngokevin

r+wc

ngokevin · 2017-07-10T16:51:29Z

src/core/component.js

    var isSinglePropSchema;
-    var oldData;
    var skipTypeChecking;
+    var oldData = this.oldData;


Can initialize this in the component constructor as undefined.

ngokevin · 2017-07-10T16:51:42Z

src/core/component.js

      delete el.initializingComponents[this.name];
-      // Play the component if the entity is playing.
+      // We pass empty object to multiple property schemas and single property schemas that parse to objects like position, rotation, scale
+      // undefined is passed to the


This comment stopped suddenly

ngokevin · 2017-07-10T16:52:35Z

src/core/component.js

      this.update(oldData);
+      // Play the component if the entity is playing.
      if (el.isPlaying) { this.play(); }
+      this.oldData = extendProperties({}, this.data, isSinglePropSchema);


// Store current data as previous data for future updates.

ngokevin · 2017-07-10T16:53:09Z

src/core/component.js

-      if (!skipTypeChecking && utils.deepEqual(oldData, this.data)) { return; }
+      if (!skipTypeChecking && utils.deepEqual(this.oldData, this.data)) { return; }

+      this.oldData = extendProperties({}, this.data, isSinglePropSchema);


// Store current data as previous data for future updates.

ngokevin · 2017-07-10T16:53:52Z

src/core/component.js

    // 1. Default values (lowest precendence).
    if (isSinglePropSchema) {
-      data = schema.default;
+      data = typeof schema.default === 'object' ? utils.extend({}, schema.default) : schema.default;


Clone default value if object so components don't share object.

ngokevin · 2017-07-10T16:54:23Z

Can also update the PR description for future reference.

…te on component initialization (fix aframevr#2829)

handle undefined oldData initially (fixes side effect of aframevr/aframe#2840)

nwoltman · 2017-07-19T04:02:29Z

Is there going to be a 0.6.1 patch release for this bug fix?

ngokevin · 2017-07-19T04:02:56Z

yeah

…te on component initialization (fix #2829) (#2840)

handle undefined oldData initially (fixes side effect of aframevr/aframe#2840)

dmarcos added the needs review label Jul 4, 2017

dmarcos force-pushed the fix2829 branch from 0b2653a to 8f7b79c Compare July 4, 2017 07:13

dmarcos added the needs changes label Jul 6, 2017

dmarcos force-pushed the fix2829 branch 2 times, most recently from 71645a7 to f0f6ff6 Compare July 8, 2017 06:19

dmarcos removed the needs changes label Jul 8, 2017

ngokevin approved these changes Jul 10, 2017

View reviewed changes

ngokevin added needs changes and removed needs review labels Jul 12, 2017

dmarcos force-pushed the fix2829 branch from f0f6ff6 to a779c9d Compare July 12, 2017 22:51

dmarcos added needs review and removed needs changes labels Jul 12, 2017

Store oldData after building component data. undefined passed to upda…

6ad30a9

…te on component initialization (fix aframevr#2829)

dmarcos force-pushed the fix2829 branch from a779c9d to 6ad30a9 Compare July 13, 2017 02:39

dmarcos merged commit d6d6201 into aframevr:master Jul 14, 2017

dmarcos mentioned this pull request Jul 14, 2017

setAttribute on text geometry not working #2837

Closed

feiss added a commit to supermedium/aframe-environment-component that referenced this pull request Jul 16, 2017

Merge pull request #12 from chenzlabs/master

650e792

handle undefined oldData initially (fixes side effect of aframevr/aframe#2840)

ngokevin pushed a commit that referenced this pull request Jul 19, 2017

Store oldData after building component data. undefined passed to upda…

921f65f

…te on component initialization (fix #2829) (#2840)

PiTiLeZarD pushed a commit to PiTiLeZarD/aframe-environment-component-v2 that referenced this pull request May 11, 2022

Merge pull request supermedium#12 from chenzlabs/master

52c2172

handle undefined oldData initially (fixes side effect of aframevr/aframe#2840)

Uh oh!

Store oldData after building component data. undefined passed to upda… #2840

Store oldData after building component data. undefined passed to upda… #2840

Uh oh!

Conversation

dmarcos commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngokevin commented Jul 5, 2017

Uh oh!

dmarcos commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngokevin commented Jul 5, 2017

Uh oh!

dmarcos commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngokevin commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmarcos commented Jul 6, 2017

Uh oh!

ngokevin left a comment

Choose a reason for hiding this comment

Uh oh!

ngokevin Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

ngokevin Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

ngokevin Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

ngokevin Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

ngokevin Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

ngokevin commented Jul 10, 2017

Uh oh!

nwoltman commented Jul 19, 2017

Uh oh!

ngokevin commented Jul 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmarcos commented Jul 4, 2017 •

edited

Loading

dmarcos commented Jul 5, 2017 •

edited

Loading

dmarcos commented Jul 5, 2017 •

edited

Loading

ngokevin commented Jul 5, 2017 •

edited

Loading