Skip to content
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

Animation API refactor #420

Merged
merged 21 commits into from
Sep 21, 2016
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions framer/Animation.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -44,30 +44,30 @@ class exports.Animation extends BaseClass
# properties:
# x: 100

# print args

layer = null
properties = {}
options = {}

# Actual current api
if arguments.length is 3
layer = args[0]
properties = args[1]
options = args[2]

# Mix of current and old api
if arguments.length is 2
layer = args[0]
if args[1].properties
if args[1].properties?
properties = args[1].properties
else
properties = args[1]
options = args[1].options if args[1].options

options = args[1].options if args[1].options?

# Old api
if arguments.length is 1
layer = args[0].layer
properties = args[0].properties
if args[0].options
if args[0].options?
options = args[0].options
else
options = args[0]
Expand All @@ -76,9 +76,6 @@ class exports.Animation extends BaseClass
delete options.properties
delete options.options


# print "Animation", layer, properties, options

@options = _.cloneDeep(Defaults.getDefaults("Animation", options))

super
Expand Down Expand Up @@ -162,6 +159,11 @@ class exports.Animation extends BaseClass
console.log "Animation.start"
console.log "\t#{k}: #{@_stateA[k]} -> #{@_stateB[k]}" for k, v of @_stateB

# Add the callbacks
@on(Events.AnimationStart, @options.onStart) if _.isFunction(@options.onStart)
@on(Events.AnimationStop, @options.onStop) if _.isFunction(@options.onStop)
@on(Events.AnimationEnd, @options.onEnd) if _.isFunction(@options.onEnd)

# See if we need to repeat this animation
# Todo: more repeat behaviours:
# 1) add (from end position) 2) reverse (loop between a and b)
Expand Down
1 change: 1 addition & 0 deletions framer/Extras/TouchEmulator.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ class TouchEmulator extends BaseClass
curve: "ease-out"

hideTouchCursor: ->
return unless @touchPointLayer.opacity > 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in this PR?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-/

@touchPointLayer.animateStop()
@touchPointLayer.animate
opacity: 0
Expand Down
41 changes: 18 additions & 23 deletions framer/Layer.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Utils = require "./Utils"
{Matrix} = require "./Matrix"
{Animation} = require "./Animation"
{LayerStyle} = require "./LayerStyle"
{LayerStateMachine} = require "./LayerStateMachine"
{LayerStates} = require "./LayerStates"
{LayerDraggable} = require "./LayerDraggable"
{LayerPinchable} = require "./LayerPinchable"
{Gestures} = require "./Gestures"
Expand Down Expand Up @@ -133,7 +133,6 @@ class exports.Layer extends BaseClass
@[p] = options[p]

@animationOptions = {}
@_stateMachine = new LayerStateMachine(@)
@_context.emit("layer:create", @)

##############################################################
Expand Down Expand Up @@ -885,30 +884,25 @@ class exports.Layer extends BaseClass
##############################################################
## ANIMATION

# Used to animate to a state with a specific name
# We lookup the stateName and call 'animate' with the properties of the state
animateToState: (stateName, options={}) ->
return @_stateMachine.switchTo(stateName, options)

animate: (properties, options={}) ->

# print "layer.animate", properties, options

# If the properties are a string, we assume it's a state name
if _.isString(properties)
return @animateToState(properties, options)
# Support options as an object
options = options.options if options.options?
return @states.machine.switchTo(properties, options)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nicer to put properties in a new variable stateName here to make it more clear what is happening


# Support the old properties syntax, we add all properties top level and
# move the options into an options property.
if properties.hasOwnProperty("properties")
if properties.properties?
options = properties
properties = options.properties
delete options.properties

# With the new api we treat the properties as animatable properties, and use
# the special options keyword for animation options.
if properties.hasOwnProperty("options")
options = _.defaults(properties.options, options)
if properties.options?
options = _.defaults({}, options, properties.options)
delete properties.options

# Merge the animation options with the default animation options for this layer
Expand All @@ -920,13 +914,13 @@ class exports.Layer extends BaseClass

return animation

switchInstant: (properties, options={}) ->
@animate(properties, _.merge(options, {instant: true}))

animateToNextState: (args..., options={}) ->
stateCycle: (args..., options={}) ->
states = []
states = _.flatten(args) if args.length
@animate(@_stateMachine.next(states), options)
@animate(@states.machine.next(states), options)

stateSwitch: (stateName, options={}) ->
@animate(stateName, _.defaults({}, options, {instant:true}))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to support animated: true?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well ok.


animations: ->
# Current running animations on this layer
Expand Down Expand Up @@ -986,17 +980,18 @@ class exports.Layer extends BaseClass
enumerable: false
exportable: false
importable: false
get: -> @_stateMachine.states
get: ->
@_states ?= new LayerStates(@)
return @_states
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really dislike this behaviour of capturing the state inside of a getter, leads to unexpected behaviour. Can't we add it to the creation / addition of states?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'd like that, but:

  1. There is no hook to see if a state gets created (because it's just an object key).
  2. This guarantees backwards compat

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with 2), we could fix 1) if we ever move to initial, but am I correct that in order to drop this, we would also need to drop the backwards compatibility of default?

set: (states) ->
@_stateMachine.reset()
for name, state of states
@_stateMachine.states[name] = state
@states.machine.reset()
_.extend(@states, states)
Copy link
Collaborator

@nvh nvh Sep 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could capture the current state of the layer here, we should however be aware of the setting of states individually...

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, because this should work too:

layer = new Layer
layer.states.test = {x: 500}
layer.animate "default"


@define "stateNames",
enumerable: false
exportable: false
importable: false
get: -> @_stateMachine.stateNames
get: -> @states.machine.stateNames

#############################################################################
## Draggable, Pinchable
Expand Down
58 changes: 33 additions & 25 deletions framer/LayerStateMachine.coffee
Original file line number Diff line number Diff line change
@@ -1,47 +1,46 @@
# {_} = require "./Underscore"
#
# {Events} = require "./Events"
{BaseClass} = require "./BaseClass"
# {Defaults} = require "./Defaults"

{LayerStates} = require "./LayerStates"

class exports.LayerStateMachine extends BaseClass

constructor: (layer) ->
constructor: (@_layer, @_states) ->
super
@_layer = layer
@properties = {}
@initial = LayerStates.filterStateProperties(layer.props)

@reset()

@define "layer",
get: -> @_layer

@define "current",
get: -> @states[@currentName]
get: -> @currentName

@define "previous",
get: -> @previousName


@define "currentName",
get: -> @_currentName

@define "previous",
get: -> @states[@previousName]

@define "previousName",
get: -> _.last(@_previousNames)
get: -> _.last(@_previousNames) or "default"
Copy link
Collaborator

@nvh nvh Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? (I don't understand the case where _.last(@_previousNames) is null)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to always return something here so you don't have to do a null check (if you just created a layer and ask for layer.states.previous.x.


@define "stateNames",
get: -> _.keys(@states)
get: -> Object.keys(@states)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use Object.keys and not _.keys here?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are both equal, and I want to be sure Object.keys is the exact same as doing it on the layer.states.


@define "states",
get: -> @_states

switchInstant: (stateName) ->
@switchTo(stateName, {instant: true})

switchTo: (stateName, options={}) ->

# Check if the state exists, if not this is a pretty serious error
throw Error "No such state: '#{stateName}'" unless @states.hasOwnProperty(stateName)
throw Error "No such state: '#{stateName}'" unless @states[stateName]

# Prep the properties and the options. The options come from the state, and can be overriden
# with the function arguments here.
properties = @states[stateName]
options = _.defaults(properties.options, options) if properties.options
options = _.defaults({}, options, properties.options) if properties.options

stateNameA = @currentName
stateNameB = stateName
Expand Down Expand Up @@ -84,12 +83,21 @@ class exports.LayerStateMachine extends BaseClass
states = @stateNames
Utils.arrayNext(states, @currentName)

emit: (args...) ->
super
# Also emit this to the layer with self as argument
@_layer.emit args...

reset: ->
@states = new LayerStates(@)

for k in _.keys(@states)
delete @states[k] unless k is "default"

@_previousNames = []
@_currentName = _.first(@stateNames)
@_currentName = "default"

emit: (args...) ->
super
# Also emit this to the layer with self as argument
@_layer.emit args...
# _namedState: (name) ->
# return _.extend(_.clone(@states[name]), {name: name})

toInspect: (constructor) ->
return "<#{@constructor.name} id:#{@id} layer:#{@layer.id} current:'#{@currentName}'>"
Loading