adds the feature to animate a color property on the a-animation compo… #1302

JohnRodney · 2016-03-30T20:20:29Z

Description:
Adds the support for using a-animation components in order to animate colors. #733
Changes proposed:

Adds the detection of a color as an attribute
Adds a partialSetAttribute for color
Adds a conversion from rgb object to hex (maybe these belong in utils?)
Adds two unit tests for start and finish to ensure the color is properly tweened

…nent

ngokevin · 2016-03-30T20:26:29Z

src/utils/coordinates.js

 */
 module.exports.isCoordinate = function (value) {
-  return regex.test(value);
+  return value[0] !== '#' && regex.test(value);


Color hexes were passing the regex test?

Yes I'm not really a regex wizard so I wasn't sure how to adjust it to return false if a hex value was passed. Probably would be cleaner to adjust the regex initialization than check for a hash as index 0.

So coordinates will always have whitespaces in between the numbers. Colors won't. I see that the whitespace characters uses star (*) rather than (+) which will match even 0 whitespace characters. Need to change those to + to force at least 1 whitespace character.

BEFORE: /\s*(-?\d*\.{0,1}\d+)\s*(-?\d*\.{0,1}\d+)\s*(-?\d*\.{0,1}\d+)\s*/;
AFTER:: /\s*(-?\d*\.{0,1}\d+)\s+(-?\d*\.{0,1}\d+)\s+(-?\d*\.{0,1}\d+)\s*/;

This has been updated as well thanks for the help on the regex.

…d a new test that checks start inbetween and end for shorthand and capital from and to values

…es the index 0 check for hash characters

dmarcos · 2016-03-30T21:24:14Z

src/core/a-animation.js

+ * @returns {string}
+ */
+function componentToHex (color) {
+  var hex = color.toString(16);


Should we check if color < 0 or > 255 and clamp it to 0-255?

@dmarcos @ngokevin Hey this could be clamped in the function that calls this by doing a:
return Math.floor(Math.min(Math.abs(color), 1) * 255);
inside of the convertToIntegerColor() function. Would this be acceptable? Or would you prefer a separate clamp function?

This is also possibly unneeded as the value that is passed comes from a THREE.Color which is already tested to be within 0 and 1 from a hex value correct?

… in only values from 0 - 255 and adjusts the import three to import from lib/three

…ture

JohnRodney · 2016-03-31T00:45:12Z

I think all feedback is resolved and I went through a refactor phase. It should be ready to squash the commits if everything else is approved.

ngokevin · 2016-03-31T01:02:15Z

@dmarcos This looks pretty good. He made it not explicitly check for color, just let it fall through the if/else statements, saving about probably 50 LOC. I think even if we add more interpolation types, this should still hold.

dmarcos · 2016-03-31T01:41:53Z

src/core/a-animation.js

    getForCoordinateComponent();
  } else if (['true', 'false'].indexOf(dataTo) !== -1) {
    getForBoolean();
+  } else if (isNaN(dataTo)) {


Does this work for the following case? Can we add it to the tests?

<a-animation attribute="material.color" from="red" to="blue"></a-animation>

Good catch. I'll look at it in the morning.

…x refactored to dry up code and more accurately name itself

… animation

ngokevin · 2016-03-31T03:44:01Z

src/core/a-animation.js

      from[attribute] = dataFrom;
    }
    from[attribute] = parseProperty(from[attribute], schema[componentPropName]);
    to[attribute] = parseProperty(dataTo, schema[componentPropName]);


If our color property type parser converted to THREE.Color I think everything would work out of the box. But I'm not sure if that would break things.

JohnRodney · 2016-03-31T20:19:31Z

@ngokevin @dmarcos Updated with a new approach to support both color and material.color. I feel better about this approach. Will add in some unit tests to ensure no regressions are introduced around the 'material.color' property.

ngokevin · 2016-03-31T20:24:03Z

src/core/a-animation.js

  var to = {};
-
-  if (attributeSplit.length === 2) {
+  if (attributeSplit.length === 2 && attributeSplit.indexOf('color') === -1) {


Note that material.color is only a single test case. Properties can be named anything and still be of type color. I'll try to help find a solution.

are you saying something with a color property could be passed that did not contain the word color in it?

Yup. Anyone can create components with whatever schema they want. So I could do:

schema: { body: {type: 'color'} }

Ok so exact matching the schema type should be safe here and then the rest should still work as far as the tweens are concerned. I'm using the passed attributes to set when there are more than one in attributeSplit

Ah, that's interesting. Seems like that might work. It's not ideal we need to special-case colors, but I'm feel okay with this.

You are right that will support both. And make all the test cases for color animation currently commited still work.

Then we would just have to figure out how to add el.components.material.schema.color.type = 'color' to the generated entity to test the cases of material.color or material.specular

You don't have to add anything. Just set the material component onto the entity.

el.setAttribute('material', 'shader: flat; color: #FFF');

Now that I look at it I had tried that method earlier but it breaks 4 tests because I'm guessing in certain test cases the mapping doesn't exist so its not safe to check for it as the tests will try to call a property on an undefined object.

I'd ignore everything about mappings. That's part of what is currently not core code (e.g., <a-box>). Though it should be able to interpolate attributes that it detects are colors (e.g., color="#FFFFFF", but that doesn't require looking up schema.

Alright took your suggestion and pushed another version up with the fallback to isNaN and a isColor function that checks to make sure schemas are defined so that older tests don't fail with this introduction.

…the animation type. Still needs new unit tests

dmarcos · 2016-04-01T15:53:55Z

src/core/a-animation.js

+   * Match the sceme type to color
+   * @return {bool} if the schema is of type color
+   */
+  function isColor () {


You can clean this up a little bit:

function isColor () { var componentName = attributeSplit[0]; var propertyName = attributeSplit[1]; var component = el.components[componentName]; var schema = component && component.schema; if (schema) { return schema[propertyName].type === 'color' } return true; }

JohnRodney · 2016-04-20T16:10:21Z

@dmarcos We all do here too haha.

@ngokevin I adjusted the requested changes. The only thing that may be missing I can think of now is a link to the example on the index page that is on localhost:9000 when you start the server. I'm guessing I should probably add this yes?

ngokevin · 2016-04-20T21:52:04Z

docs/core/animations.md

+
+```html
+<a-entity id="blushing-cube" geometry="primitive: box">
+  <a-animation attribute="color" begin="white" to="red" dur="1000"></a-animation>


Should be attribute="material.color"

begin="white" should be from="white"

ngokevin · 2016-04-20T21:54:11Z

OK, that should be the last of it!

JohnRodney · 2016-04-22T18:29:42Z

@ngokevin alright I updated the demo to a little more fancy of a demo. I also opened an issue because you can't use begin delays to stagger because they calculate the begin delay on a repeat meaning the "stagger" animation eventually gets out of sync.

This can be updated to remove the intervals once the bug is fixed there.

ngokevin · 2016-04-22T18:49:22Z

Awesome! I'll merge this once I get to the office.

JohnRodney · 2016-04-22T19:09:55Z

Cool once you give it one more look over and are ready let me know and I'll squash the commits

ngokevin · 2016-04-22T19:44:39Z

examples/test-animate-color/index.html

+        <a-animation attribute="color" dur="10000" from="#afafaf" to="#000000" repeat="indefinite">
+      </a-sky>
+
+      <a-entity position="4.5 1 -3" class="pulse" geometry="primitive: box; depth: 0.5; height: 1; width: 0.5" material="color: #5698Af">


You can use mixins to clean up.

<a-assets> <a-mixin id="box" geometry="primitive: box..." material="..."></a-mixin> <a-mixin id="pulse" attribute="material.color" from="#fff" to="#5698Af" dur="1000" begin="pulse"></a-mixin> </a-assets> <a-entity mixin="box"><a-animation mixin="pulse"></a-animation></a-entity> <a-entity mixin="box"><a-animation mixin="pulse"></a-animation></a-entity>

JohnRodney · 2016-04-22T20:47:29Z

@ngokevin adjusted to mixins, new file structure and stats attribute.

ngokevin · 2016-04-22T20:48:47Z

examples/test/animate-color/index.html

+        <a-animation attribute="color" dur="10000" from="#afafaf" to="#000000" repeat="indefinite">
+      </a-sky>
+
+      <a-entity position="4.5 1 -3" class="pulse" mixin="box"><a-animation mixin="pulse"></a-animation></a-entity>


You don't need the class anymore because your selectorAll can be [mixin="box"]

ngokevin · 2016-04-22T21:17:24Z

Thanks so much!!! That was a long process, but everything works, and your example is pretty cool and running at 60fps :)

adds the feature to animate a color property on the a-animation compo…

b3fb5a8

…nent

ngokevin added the needs review label Mar 30, 2016

ngokevin reviewed Mar 30, 2016
View reviewed changes

JohnRodney added 2 commits March 30, 2016 16:01

adds 4 new test conditions: inbetween test for initial color test, an…

4b4e32b

…d a new test that checks start inbetween and end for shorthand and capital from and to values

update regex assignment to fail when spaces are not present and remov…

3987a47

…es the index 0 check for hash characters

dmarcos reviewed Mar 30, 2016
View reviewed changes

JohnRodney added 4 commits March 30, 2016 16:47

clamp the color value from 0 - 1 so multiplication by 255 will result…

444a3b7

… in only values from 0 - 255 and adjusts the import three to import from lib/three

adds support for hsl, rgb, and color nouns in a-animation component

3cf6064

adds a test generator to dry up testing code around animate color fea…

be448d0

…ture

refactor deprecated arguments

1ab765a

dmarcos reviewed Mar 31, 2016
View reviewed changes

JohnRodney added 2 commits March 30, 2016 20:54

refactor: A logic edge cause was repaired in the color clamp, rgbToHe…

70dae49

…x refactored to dry up code and more accurately name itself

resolves the issue with material.color failing as a properly to color…

26fabac

… animation

ngokevin reviewed Mar 31, 2016
View reviewed changes

dmarcos added needs changes and removed needs review labels Mar 31, 2016

change the approach to color tween on attribute splits

aa564e0

ngokevin reviewed Mar 31, 2016
View reviewed changes

JohnRodney added 2 commits March 31, 2016 21:06

adjusts to use a true special-case color check in order to determine …

fb53ca5

…the animation type. Still needs new unit tests

refactor to supportial multiple attribute colors and fixes the tests.

5447365

dmarcos reviewed Apr 1, 2016
View reviewed changes

ngokevin added needs review and removed needs changes labels Apr 20, 2016

ngokevin reviewed Apr 20, 2016
View reviewed changes

ngokevin added needs changes and removed needs review labels Apr 20, 2016

JohnRodney added 4 commits April 21, 2016 10:34

update doc typo

49bf745

adjust the wording in the doc around collor properties

b2051c4

change to a pulse based demo

0218626

merge master into branch

82cb9ac

ngokevin reviewed Apr 22, 2016
View reviewed changes

fit the new file structure and dry up code with mixins

db60d0f

ngokevin reviewed Apr 22, 2016
View reviewed changes

JohnRodney added 2 commits April 22, 2016 16:08

remove unneeded class

608b6a8

remove unneeded class

cf2ce11

ngokevin removed the needs changes label Apr 22, 2016

ngokevin merged commit 29446e0 into aframevr:master Apr 22, 2016

ngokevin mentioned this pull request Apr 22, 2016

Support animating colors #733

Closed

Uh oh!

adds the feature to animate a color property on the a-animation compo… #1302

adds the feature to animate a color property on the a-animation compo… #1302

Uh oh!

Conversation

JohnRodney commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnRodney commented Mar 31, 2016

ngokevin commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnRodney commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnRodney commented Apr 20, 2016

Choose a reason for hiding this comment

ngokevin commented Apr 20, 2016

JohnRodney commented Apr 22, 2016

ngokevin commented Apr 22, 2016

JohnRodney commented Apr 22, 2016

ngokevin Apr 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

JohnRodney commented Apr 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin commented Apr 22, 2016

Labels

4 participants

ngokevin Apr 22, 2016 •

edited

Loading