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

Calculate distance correctly for animations using byValue #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

@@ -9,7 +9,7 @@

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.

@@ -244,7 +244,7 @@ public extension SKAction {
if initialValue == nil {

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.

None yet

2 participants