Skip to content

Conversation

noahsark769
Copy link

@noahsark769 noahsark769 commented May 25, 2017

Hey! Awesome library, thanks for building it. I noticed that there's a bug with animations that use byValue, e.g.:

SKAction.scale(by:duration:delay:usingSpringWithDamping:initialSpringVelocity:)

The distance isn't calculated correctly, so the behavior uses byValue as the distance.

For example, say there's a node with a current scale of 1.0 - if we tried to scale it by 1.2, SpriteKit-Spring will currently scale it to 1.0 + 1.2 instead of 1.0 * 1.2, which in this case would more than double the scale instead of scaling appropariately. I put up an example app at https://github.com/noahsark769/NGSpriteKitSpringTest to demonstrate this.

The fix is to calculate the initialDistance based on multiplying it by initialValue.

Let me know if I need to do anything else to fix this bug! Thanks!

@noahsark769
Copy link
Author

@ataugeron bump! Would be awesome to get this out and pushed as a new version of the cocoapod 👍

Copy link
Owner

@ataugeron ataugeron left a comment

Choose a reason for hiding this comment

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

In addition to the inline comments, I merged #16 so we should make sure this CL is compatible.

Pod::Spec.new do |s|
s.name = "SpriteKit-Spring"
s.version = "1.0.1"
s.version = "1.1.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Let's not mix code changes with version bumps in the same PR. I'll take care of bumping the version and updating the pod once all PRs are merged.


initialValue = node.value(forKeyPath: keyPath) as! CGFloat
initialDistance = initialDistance ?? finalValue - initialValue!
initialDistance = initialDistance != nil ? initialDistance * initialValue - initialValue : finalValue - initialValue!
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for catching this issue. However I don't think this is the correct fix. It appears the behavior of the "by" param in SKAction is inconsistent:
SKAction.scaleX(by: 0.5, y: 0.5, duration:0) // will divide xScale and yScale by 2
SKAction.moveBy(x: 0.5, y: 0.5, duration: 0) // will add 0.5 to position.x and position.y

For this reason I don't think we should change the behavior of the animate(keyPath:...) functions, but just adjust the behavior of the scaleBy(...) functions above to compute the correct byValue to give to animate(keyPath:...).

Does this make sense?

Copy link
Author

Choose a reason for hiding this comment

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

Wow you're right, great catch! I agree, since scale is the only multiplicative SKAction we should handle it as a special case in the scaleBy functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants