Skip to content
This repository has been archived by the owner on Jun 7, 2018. It is now read-only.

Add AnimationEffect.composite #13

Closed
wants to merge 1 commit into from

Conversation

steveblock
Copy link
Contributor

Both KeyframeAnimationEffect and PathAnimationEffect have a composite
attribute. This change adds the attribute to their common base class,
AnimationEffect.

Both KeyframeAnimationEffect and PathAnimationEffect have a composite
attribute. This change adds the attribute to their common base class,
AnimationEffect.
@birtles
Copy link
Contributor

birtles commented Jun 24, 2013

This was deliberate because in the model the composition operation is specific to each type of animation effect since we imagined there would be types of effects where addition didn't make sense. Do you think it's better to put it on the base class and split it off later if necessary?
(Note that the meaning is slightly different since in KeyframeAnimationEffect this member represents the 'default' composition operation)

@steveblock
Copy link
Contributor Author

This was deliberate because in the model the composition operation is specific to each type of animation effect
since we imagined there would be types of effects where addition didn't make sense.
Makes sense, but I'm not sure why this applies to composite but not accumulate (which is on AnimationEffect)?

Do you think it's better to put it on the base class and split it off later if necessary?
Not sure, but it seems odd that it's inconsistent with accumulate.

(Note that the meaning is slightly different since in KeyframeAnimationEffect this member represents the 'default'
composition operation)
True. In fact, I think the latest proposal for the new keyframe syntax removes composite from KeyframeAnimationEffect and establishes a default value for the per-keyframe composite.

WDYT?

@steveblock
Copy link
Contributor Author

The plan is to move the accumulate attribute to the derived classes to match composite - https://www.w3.org/Bugs/Public/show_bug.cgi?id=22440

@steveblock steveblock closed this Jun 25, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants