Substantially optimize coordinates.parse

[mqp]

coordinates.parse winds up accounting for a moderate amount of time if you do per-frame updates of attributes containing new string or object coordinates. (In our project, an example was a raycaster component which we updated with a new direction and origin each frame.) This change makes it between 2x and 10x faster in both the string-input and object-input cases in Firefox and Chrome (depending on input characteristics) while being more or less equally straightforward.

Here's a JSPerf comparing the old and new versions.

There's one edge case I'm worried about. In the old version, if you gave the input {x: 1, y: "2", splatoon: "42", "whiz": "bang"}, it would return {x: 1, y: 2, splatoon: 42, whiz: NaN}. That is, it would parse any string keys at all in the input, even ones that it didn't know anything about, and even ones that aren't floats. That behavior seems strange to me, it didn't seem documented anywhere, and no test was exercising it, but it's true. If that behavior is important, then let me know and I'll hack it back in.

[dmarcos]

Probably more clear if you do if (coordinate[i] !== undefined)

[dmarcos]

Early return preferable:

  if (defaultVal === undefined) { continue; }
  vec[key] = parseIfString(defaultVal); 

[dmarcos]

A bit more compact using forEach

coordinateKeys.forEach(function (key, i) {
   if (coordinate[i] !== undefined) {
      vec[key] = parseFloat(coordinate[i], 10); 
   } else {
      if (defaultVal === undefined) { return; }
      vec[key] = parseIfString(defaultVal); 
  }
}

[dmarcos]

Capitalize since it's a const COORDINATE_KEYS

[dmarcos]

Some of the unit tests are failing with the change.

[dmarcos]

Thanks for the PR. It looks good. One thing that you might want to consider. If you are calling setAttribute on a tick I recommend reusing the same object, the values will be considered trusted and type checking / parsing skipped altogether (https://aframe.io/docs/0.8.0/introduction/best-practices.html#tick-handlers) also reducing object allocation. For instance:

init: function () {
 this.yourComponentData = {coords: {x: 0, y: 0, z:0}, enabled: true};
},
tick: function () {
  this.yourComponentData.coords.x += 1;
  this.el.setAttribute('your-component', this.yourComponentData);
}

[ngokevin]

If you are in a tick, better to just avoid setAttribute altogether. We should have the raycaster component update the origin/direction directly. We already do the object reuse, that only saves on parsing.

[mqp]

I made the style changes you suggested. The test failure was a real bug; it was the new version doing a bad job of handling the empty string or a string with all spaces as input. It should line up now. Performance still seems equally good.

Thanks for the advice about reusing the same object, that advice could really be worth its weight in gold for us -- if not here, in other places. I'm not sure anyone on our team realized that!

[dmarcos]

Thank you. First PR achievement unlocked 🎉 Congrats!