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 support for simple image animations #219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dowski
Copy link

@dowski dowski commented Feb 16, 2020

This is one way of addressing #43.

Sequences of images can be changed one after another to perform simple animations. Animations can be tied to the x or y position of an Actor or directly to the pgzero clock. That allows automatic animation while an Actor moves or while time progresses in the game. They can also be driven manually (e.g. by key presses or any other in-game event).

I added an example that shows various use of the API as well as a couple of unit tests. I think more test coverage is definitely in order if we're going to add this to pgzero, but I wanted some feedback on the current state before going further.

I'm also up for writing documentation for this before merging if it seems like a good fit for the framework.

Sequences of images can be changed one after another to perform simple
animations. Animations can be tied to the x or y position of an Actor or
directly to the pgzero clock. That allows automatic animation while an
Actor moves or while time progresses in the game.
@lordmauve
Copy link
Owner

This is pretty good. It explores a lot of concepts.

A few gut feels:

  • I like the idea of an animation as an infinite cycle of frames.
  • I think you mostly nail the feature set this should have.
  • animate.images() feels like the wrong name. It doesn't exactly nail how this relates to actors.
  • I would prefer pause()/play() to start()/stop(). In particular, stop() usually indicates resetting the current frame.
  • A functional API also feels wrong given how overloaded it is. I think maybe we need to expose the ImageAnimation class.
  • I think alt_images is not the right solution; if you're looking for left/right flips then that's a feature that is already in development for Actor (iirc none of the pull requests for this (Fixed #36 - Allow assignment to Actor's width and height to scale the Actor #92, Added support for flipping Actor images horizontally and vertically. #116) are quite landable yet because they mess up positioning).
  • Linking directly to actor x and actor y feels a bit too magical. It's nice for me as a game dev but I think that for beginners it's better to be a bit lower level, so they can see how things work and understand the value of abstractions when they create them or come across them. Maybe there's a simple call we can add to the update() function to wire this up, eg. anim.jump(actor.x // 20). Linking animations to the clock is very much expected, though.
  • I guess you chose every because it works for time and coordinates. If we have an API that is specific to time I'd prefer a parameter name that is specific to time, like rate, frame_rate, fps or something. This makes code more readable: every=20 is ambigious about the units for the 20, while fps=20 is not.

Something I'd like to add is sprite sheets, where there are many sprites in one source image. That will be easier to overload if we have a class. Then we can just add class methods for different ways of constructing the sequence. eg. ImageSequence.from_sprite_grid('spritesheet', rows=1, columns=10). (This is needed partly because many of the animations you can find on the web are in this form).

I think with a class we can sort out the overloading of the constructor by creating different methods, like play_once(), play_looping() etc.

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