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

Add FlxTween.shake for sprite shaking. #2549

Merged
merged 3 commits into from
May 8, 2022
Merged

Add FlxTween.shake for sprite shaking. #2549

merged 3 commits into from
May 8, 2022

Conversation

SavanDev
Copy link
Contributor

@SavanDev SavanDev commented May 2, 2022

Implement the same function that exists in FlxCamera to be used on an individual FlxSprite (and not on the whole screen).
Same as #2548 but now from FlxTween:

FlxTween.shake(Sprite, Intensity, Duration, Axes, Options);

(I did it based on the tweens that are already made to work the same way)

Here two examples:

1.mp4
2.mp4

@ghost
Copy link

ghost commented May 2, 2022

yes

if (axes != Y)
sprite.x = initialXY.x + FlxG.random.float(-intensity * sprite.width, intensity * sprite.width);
if (axes != X)
sprite.y = initialXY.y + FlxG.random.float(-intensity * sprite.height, intensity * sprite.height);
Copy link
Member

Choose a reason for hiding this comment

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

rather than storing the initial x and y, I'm wondering if it should store the sprite's offset, and use the offset to shake it. this would allow it to shake objects in motion.

Also initialXY doesn't fit the name scheme, it should be initialPosition or (if changed) initialOffset

@Geokureli
Copy link
Member

Geokureli commented May 2, 2022

If this tween is cancelled will it be reverted back to its original position, or the position it was shaken to? I'd prefer the former.
I know this happens if its finished, but this is a special case

We should add unit tests that check both, and perhaps other test are in order. Tests can be added here:
https://github.com/HaxeFlixel/flixel/blob/dev/tests/unit/src/flixel/tweens/FlxTweenTest.hx

Copy link
Member

@Geokureli Geokureli left a comment

Choose a reason for hiding this comment

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

Unless you have a good argument against it, this should use sprite.offset rather than the position directly. this will also ensure that the shake effect doesn't interfere with collision bounds and is a purely graphical effect.

WIth that, renaming initialXY, and a unit test that ensures offset is properly reset on complete and cancel, this should be good to go

@SavanDev
Copy link
Contributor Author

SavanDev commented May 4, 2022

Now as soon as I have some time (probably at the weekend) I will start to make the changes in the code and the tests to check that it works well.
Sorry for not replying sooner.

@SavanDev
Copy link
Contributor Author

SavanDev commented May 8, 2022

I have tried to run the unit test but I am not very familiar with it yet.
If you see that I'm missing something or it's done wrong let me know and I'll correct it (what I've tried passes the test).

Other than that, I've been testing it on a new project to see if it actually works correctly when finalizing and/or canceling the tween and I haven't seen any problems with either.
I uploaded it to GitHub so you can see it: https://github.com/SavanDev/ShakeTest

@Geokureli
Copy link
Member

Geokureli commented May 8, 2022

For some reason I'm not seeing utnit test outcomes in github actions, so I checked yours locally and it looks good.

Thanks for this!

P.S.: I'm making a change to FlxAxes in another branch, so you'll see a similar change to ShakeTween, from me, pretty soon

@Geokureli Geokureli merged commit 0fe94ad into HaxeFlixel:dev May 8, 2022
@hayfidev
Copy link

hayfidev commented May 28, 2022

this got added right? i cant use it
Edit: Nvm I figured it out :D

@Geokureli
Copy link
Member

Its not released yet, youll need to use haxelib dev or haxelib git

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.

3 participants