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

Possibility to transfer the current_frame field between animations #168

Closed
VictorKoenders opened this issue Apr 10, 2020 · 5 comments · Fixed by #169
Closed

Possibility to transfer the current_frame field between animations #168

VictorKoenders opened this issue Apr 10, 2020 · 5 comments · Fixed by #169
Labels
Area: Graphics Issues related to graphics/rendering. Type: Feature Request Improvements that could be made to the code/documentation.

Comments

@VictorKoenders
Copy link
Contributor

In a game I'm making I have several animations that are needed for animating a player's character whenever they're moving in a certain direction. Whenever a player switches direction by pressing a different combination of arrow keys, a new animation gets selected.

Code
pub struct Player {
    pub running_map: DirectionMap<Animation>,
   ...
}

impl Player {
    pub fn set_walking(&mut self, diff: Vec2<f32>) {
        if let Some(direction) = Direction::from_diff(diff) {
            self.pos += diff;
            self.moving = true;
            if direction != self.direction {
                self.direction = direction;
                self.running_map[self.direction].restart();
            }
        } else {
            self.set_idle();
        }
    }
}

#[derive(Debug, Clone)]
pub struct DirectionMap<T> {
    pub up: T,
    pub up_right: T,
    pub right: T,
    pub down_right: T,
    pub down: T,
    pub down_left: T,
    pub left: T,
    pub up_left: T,
}

Because the walking animation is relatively long, it is very noticeable that the animation gets .restart() every time a player switches directions. If I get rid of .restart(), the animation becomes even more irregular.

I'd suggest one of two things:

  • Expose current_frame (maybe as read-only) and allow a developer to set the current_frame.
  • Expose an fn copy_frame_index(&mut self, other: &Animation) which copies both the current_frame and timer
    • This could possibly error if the frame_length or frames.len() is different?

Whenever we decide on an API, I can make a PR

@VictorKoenders VictorKoenders added the Type: Feature Request Improvements that could be made to the code/documentation. label Apr 10, 2020
@VictorKoenders
Copy link
Contributor Author

animation_issue

@17cupsofcoffee
Copy link
Owner

I honestly think that Animation could use a lot of rework, it's a bit clunky 😅

I think I'd prefer the current_frame API, personally - the copy_frame_index API could be built on top of that in the game's code (which would give people more control over how they handle animations that are different lengths). Something like this:

fn current_frame(&self) -> usize { ... }
fn set_current_frame(&mut self, index: usize) { ... }

I'd lean towards having set_current_frame panic if you pass in an invalid index, because it seems like it'd be annoying to have to handle errors every time you call it. Unless you can think of any cases where you'd like to be able to recover from an invalid frame index? I'm open to being convinced 😄

Also, as an aside - that GIF looks very cool :) Are you making a Factorio-like kind of game?

@17cupsofcoffee 17cupsofcoffee added the Area: Graphics Issues related to graphics/rendering. label Apr 10, 2020
@17cupsofcoffee
Copy link
Owner

Also, another side note: the source code for Animation is deliberately designed to not use any of Tetra's internal APIs - you're more than welcome to copy and paste it into your project and hack around if the built-in version can't be made to fit your needs 🙂

@VictorKoenders
Copy link
Contributor Author

Are you making a Factorio-like kind of game?

I'm using the factorio assets for development.

the source code for Animation is deliberately designed to not use any of Tetra's internal APIs

Maybe it's an idea to make a seperate crate, e.g. tetra-extras, which includes things like Animation? rocket and mio do something similar. This would also make it more clear to people that these are meant as addons, and not part of the core library.

@17cupsofcoffee
Copy link
Owner

Maybe it's an idea to make a seperate crate, e.g. tetra-extras, which includes things like Animation? rocket and mio do something similar. This would also make it more clear to people that these are meant as addons, and not part of the core library.

Yeah, I'm considering doing something like this eventually :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Graphics Issues related to graphics/rendering. Type: Feature Request Improvements that could be made to the code/documentation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants