New Fighter State Design and Gameplay Refactor

[zicklag]

OK I think this is finished!

This refactors almost everything related to the gameplay 😅. It does an enormous amount of re-organizing, and clean up in addition to implementing the new fighter state design I started to describe in #196. I think a lot of it turned out easier to understand and simpler to modify.

I'm hoping this helps make it easier to maintain and easier to continue to add features as we continue to develop.

I think the only "regression" is that enemies jump around just like the player does when you attack, instead of just standing and punching, which should probably be fixed.

I want to wait to add different enemy attacks until this gets review, and we can probably do that in a separate PR.

---

Honestly, I kind of got carried away and I'll leave it up to you guys whether or not we should merge this an whether or not we do it all at the same time, if we do. If it's better, I can break this into smaller pieces and merge it one piece at a time ( as much as possible ). Removing the State enum alone pretty much necessitates a change to everything, so it can only be broken down so far, but I did re-organize a lot of things, too, so I could break the re-organizations into separate PRs if necessary.

Summary Of Changes

@odecay this is mostly to help summarize the changes I made to aid in reviewing.

Fighter State

The fighter and enemy state management changed completely. fighter_state.rs contains the entire fighter state machine. It's absolutely possible to split this out to separate files later if it gets too big, but I think it's still pretty easy to read through right now.

It uses the flow of: Collect Transition Intents ( from user input, enemy AI, or things like collisions ) → Evaluate Apply State Transitions → Run State Handlers.

The enemy AI logic is placed separately in enemy_ai.rs.

Movement Model

I removed almost all places where we manually set an item/player translation and replaced it with a modification to a LinearVelocity component.

The movement.rs file has all of the systems that modify entity transforms based on linear and angular velocities and forces.

By having all our movement controlled by modifying velocities instead of translation, we are able to centralize the constraints, which are also placed in movement.rs. We are able to arbitrarily define new constraints on any kind of entity, be it player, fighter, item, etc. that operate on the Velocity of the entity before the the transform is applied. So we no longer have the issue of clamping fighter positions all over the place!

I also removed the move_in_arch system in favor of using forces and velocities to create the arch. I think this is cleaner, but that's subjective, so we could add move in arch again after this PR if you wanted. Though I think we might want to wait to do any more work in that direction until we figure out how we'll handle our collisions/coordinates/y-movement: #17.

Attack and Damage

I made the damage system more generic, adding a Health component, a Damageable(bool) component and DamageEvent in damage.rs. I changed the player stats to record a max_health, so that we always know how much health they can go up to, and their actual health is stored in the Health component. This will allow us to add health to all kinds of stuff, such as destructible scenery, that can be damaged by attacks, without having to specialize the attack system for what it is attacking.

The attack system doesn't change much, but is now generic over anything with Health and Damageable and attacks now have a velocity that can be used to figure out which direction and how hard an attack should push what was attacked, if applicable. This is used for knockback on the player, but would be ignored if a wall was attacked for instance.

Damage events are emitted when an attack lands, and this event is used by the knockback collector system to put the player into a knockback state when they get attacked.

Projectiles & Lifetime

I moved the ProjectileLifetime component to a generic Lifetime component, so we can use it on anything that we want to despawn on a timer.

Despawn

I removed the DespawnMarker component and the despawn_entities system, because it's effectually exactly the same as just calling commands.entity(entity).despawn_recursive() which isn't really less ergonomic ( IMO ) than commands.entity(entity).insert(DespawnMarker) and doesn't require an extra import of the DespawnMarker component.

Organization

I moved almost all remaining systems that were in main.rs to modules that made sense for them, adding plugins as necessary to each module that had systems in it.

I removed the y_sort module to move it's system and component in with the camera, and lots of other small organizations.

I think its easier to browse the codebase and find the systems that effect different parts of the game now.

Tweak To Item Handling

Instead of matching on the item name to determine behavior, I add a kind field to the item YAML definition that allows you to specify what the item does. For health items you can specify how much health it refills, for throwable items you can specify how much damage it does.

Eventually I think we will want a dynamic way to handle item kinds, similar to the player state, so that they can be scripted and we don't have to have a central list of what kinds of items there are in an Enum.

But it's a good temporary solution.

Enemies Attack Like the Player

Right now enemies and the player share the same attack, though they have different animations.

This is trivial to fix, but I didn't want to work on any more things non organizational/architectural before this is merged so that can be done afterwards.

That's it! 🙃

Other than the massive list of changes above, the rest of the game stayed the same. Enemy AI logic is the same, the player clamping logic was adapted to a constraint so trip points and camera movement all works the same. The UI didn't change. Etc.

[zicklag]

@odecay and @64kramsystem I'd like to get your review on this when you get the chance. Since so much has changed, you might find it easier to browse the repo after the changes instead of doing a side-by side diff. GitHub1s can make this easier to do in the browser: https://github1s.com/zicklag/punchy/blob/fighter-state-refactor/src. The most important file is fighter_state.rs which implements a revised version of my fighter state proposal.

Again, I know this is a lot of changes and feel free to tell me I've got to break this down into more reviewable chunks.

[edgarssilva]

I'm really liking all the changes especially the machine state for the fighter, looks very ergonomic and much more structured.

[64kramsystem]

There are two complete PRs of mine, that have been open for some time 🤔

[zicklag]

There are two complete PRs of mine, that have been open for some time.

Yeah, sorry about that. While I was busy with this one it conflicts with almost everything and my local sandbox was a mess while it was in progress so switching to testing other PRs was difficult.

I'll look at those now. 👍

[64kramsystem]

There's another bug; the enemies are now jumping when they attack.

[64kramsystem]

So!

I'm approving this so it can go in, however, I'm actually approving because it's very large, and I can't give a meaningful review, at least, without taking a very considerable time to understand it.

It's difficult to review because there are many commits, and on the other hand, when looking it in one pass, there are several large blocks that have been moved around.
I've had a look at the structure, and it's relatively monolithic, so I guess that at this point splitting it would be a very considerable effort.

I'll put my considerations about the design in a separate comment.

[64kramsystem]

On-demand flushing is very interesting. I think something like this should be published 😄 at least to make devs aware of what needs to and can be done.

A doubt I have is that, since (I presume that) this design deals only with fighter states, in the future at least, the other states will need commands flushing handling. However, I guess that it's assumed that non-fighter-state-related systems will be a very small minority, therefore, easy to handle, so I think that works.

On the other hand, this is a quite custom approach to commands flushing, which I think it has some downsides:

• it will be less intuitive to understand for devs not familiar with it
• it may be made obsolete by new Bevy versions, if they make available a related convenient API (but it seems that the upcoming one won't be so)
• it may need update work on new Bevy versions (I don't know how stable is the internal interface used)

That's all. Based on my understanding of the last commits, the Y-rise issue stands, but anyway, I think that, at this point, it can be worked on it seperately, so that this PR goes in.

Generally speaking, I'm... underwhelmed, by the large amount of work required to manage storage sinchronization, even for a relatively simple game (less that 5k lines, counted by cloc), when using a parallel+asynchronous ECS (ie. Bevy). This is not related to anything in our game - it's my general impression of game engines design.

[zicklag]

I'm actually approving because it's very large, and I can't give a meaningful review, at least, without taking a very considerable time to understand it.

Yeah, I'm realizing it would have been good for me to give a summary walkthrough to help reviewing. I'll do that in another comment below. You don't have to go back and look at anything, but @odecay might find it helpful.

Also, if you notice anything I changed later that you don't like, feel free to open a PR to fix it, or an issue to discuss. Sorry this turned out so massive!

There's another bug; the enemies are now jumping when they attack.

That's a simple fix that we can do after merge. I wanted to avoid adding more gameplay code that could easily come after, so I just shared the attack system for the enemies with the players.

On-demand flushing is very interesting. I think something like this should be published smile at least to make devs aware of what needs to and can be done.

I know I was thinking that would probably make a good crate. I think the idea is somewhat similar to the new command flushing design in the Bevy Stageless RFC.

Since iyes_loopless is kind of a preview for stageless I wonder if they'd like the feature in their crate.

in the future at least, the other states will need commands flushing handling.

That's why CustomCommands is generic over a marker trait. We can add as many custom command queues as we want that get flushed whenever we want based on where we add the flush_custom_commands::<MyCustomCmds> system to the system schedule. So if other parts of the game need command flushing, we can just add it wherever we need it.

which I think it has some downsides

The implementation is super simple, and the only change to the systems themselves is that they call let commands = custom_commands.commands(); and use CustomCommands<Marker>, so I think it'd be really easy to migrate away if we needed to.

And if the Bevy Stageless proposal gets accepted, we can just migrate to that, as it provides the same functionality.

So if we needed to update it would probably take me less than half-an-hour to do it.

Based on my understanding of the last commits, the Y-rise issue stands,

Yep, I'm pretty sure that's a "physics" issue, because we need to decide how to handle our "up" axis, not a scheduling issue, so it shouldn't go into this PR.

[zicklag]

Summary Of Changes

@odecay this is mostly to help summarize the changes I made to aid in reviewing. Sorry I didn't do this earlier before you reviewed @64kramsystem!

Fighter State

The fighter and enemy state management changed completely. fighter_state.rs contains the entire fighter state machine. It's absolutely possible to split this out to separate files later if it gets too big, but I think it's still pretty easy to read through right now.

It uses the flow of: Collect Transition Intents ( from user input, enemy AI, or things like collisions ) → Evaluate Apply State Transitions → Run State Handlers.

The enemy AI logic is placed separately in enemy_ai.rs.

Movement Model

I removed almost all places where we manually set an item/player translation and replaced it with a modification to a LinearVelocity component.

The movement.rs file has all of the systems that modify entity transforms based on linear and angular velocities and forces.

By having all our movement controlled by modifying velocities instead of translation, we are able to centralize the constraints, which are also placed in movement.rs. We are able to arbitrarily define new constraints on any kind of entity, be it player, fighter, item, etc. that operate on the Velocity of the entity before the the transform is applied. So we no longer have the issue of clamping fighter positions all over the place!

I also removed the move_in_arch system in favor of using forces and velocities to create the arch. I think this is cleaner, but that's subjective, so we could add move in arch again after this PR if you wanted. Though I think we might want to wait to do any more work in that direction until we figure out how we'll handle our collisions/coordinates/y-movement: #17.

Attack and Damage

I made the damage system more generic, adding a Health component, a Damageable(bool) component and DamageEvent in damage.rs. I changed the player stats to record a max_health, so that we always know how much health they can go up to, and their actual health is stored in the Health component. This will allow us to add health to all kinds of stuff, such as destructible scenery, that can be damaged by attacks, without having to specialize the attack system for what it is attacking.

The attack system doesn't change much, but is now generic over anything with Health and Damageable and attacks now have a velocity that can be used to figure out which direction and how hard an attack should push what was attacked, if applicable. This is used for knockback on the player, but would be ignored if a wall was attacked for instance.

Damage events are emitted when an attack lands, and this event is used by the knockback collector system to put the player into a knockback state when they get attacked.

Projectiles & Lifetime

I moved the ProjectileLifetime component to a generic Lifetime component, so we can use it on anything that we want to despawn on a timer.

Despawn

I removed the DespawnMarker component and the despawn_entities system, because it's effectually exactly the same as just calling commands.entity(entity).despawn_recursive() which isn't really less ergonomic ( IMO ) than commands.entity(entity).insert(DespawnMarker) and doesn't require an extra import of the DespawnMarker component.

Organization

I moved almost all remaining systems that were in main.rs to modules that made sense for them, adding plugins as necessary to each module that had systems in it.

I removed the y_sort module to move it's system and component in with the camera, and lots of other small organizations.

I think its easier to browse the codebase and find the systems that effect different parts of the game now.

Tweak To Item Handling

Instead of matching on the item name to determine behavior, I add a kind field to the item YAML definition that allows you to specify what the item does. For health items you can specify how much health it refills, for throwable items you can specify how much damage it does.

Eventually I think we will want a dynamic way to handle item kinds, similar to the player state, so that they can be scripted and we don't have to have a central list of what kinds of items there are in an Enum.

But it's a good temporary solution.

Enemies Attack Like the Player

Right now enemies and the player share the same attack, though they have different animations.

This is trivial to fix, but I didn't want to work on any more things non organizational/architectural before this is merged so that can be done afterwards.

Removed GameStages

We just had a couple stages, HotReload and Animation, but I don't believe they were necessary. The CoreStages are enough when combined with .after() and .before() scheduling and it maximizes how many systems can run at the same time.

Not that we need the extra parallelism for performance, but I also think it doesn't help us to have stages that we don't need. And by only using the core stages, when reading the code, it saves having to investigate, for every custom stage we add, where that stage is inserted into the scheduler. The ordering of the core stages is apparent, and if there are any .after() or .before() constraints on a system, those are immediately apparent when the system is added.

That's it! 🙃

Other than the massive list of changes above, the rest of the game stayed the same. Enemy AI logic is the same, the player clamping logic was adapted to a constraint so trip points and camera movement all works the same. The UI didn't change. Etc.

Hopefully, that overview covers the changes well and if you can't easily review the code itself, you can at least understand whether any of those changes are something you disagree with conceptually so I can modify the PR if necessary.

[zicklag]

I just discovered that the whole custom command queues thing I made isn't even necessary! Bevy automatically flushes the command queues at the end of every SystemStage by default, unless you explicitly make a stage that does not flush the queue.

That's rather convenient!

Pushed an update that switches back to normal Commands.

[odecay]

I just discovered that the whole custom command queues thing I made isn't even necessary! Bevy automatically flushes the command queues at the end of every SystemStage by default, unless you explicitly make a stage that does not flush the queue.

That's rather convenient!

Pushed an update that switches back to normal Commands.

yep this was the intent of splitting stuff up into stages 👍
glad we can avoid the custom commands.

I think I'm starting to recover a bit from being sick so I should have a chance to review this and catch up with the ongoing discussion.

[odecay]

I intend to review code but before that

It uses the flow of: Collect Transition Intents ( from user input, enemy AI, or things like collisions ) → Evaluate Apply State Transitions → Run State Handlers.

I like this general design intent 👍

Attack and Damage

I like the damage changes, will be a useful addition for when we get to breakable items.
Other than that the attack changes seem relatively minor in their design implications which is good for this PR.

Projectiles & Lifetime

This is what I had originally implemented the lifetime for, (was called AttackLifetime before) but moved it to be projectile specific because that seemed to be the only place it made sense design wise after introducing the frame based melee attack changes.
I see it could be useful as a more general thing, but it was also used as sortof a marker component for projectiles before. Maybe if we want to use it generally it would make sense to move it out of the attacks.rs file (not sure if you did this already because I haven't looked at code yet)

Despawn

The DespawnMarker was introduced because of an issue that was occurring due to parallelism in system schedule + commands application. What was happening was that one system in a stage (the systems were all in the same stage) would add a component to an entity, and another would despawn the same entity, then when the end of stage was reached, commands would apply and non-deterministically depending on the order, if despawn was applied first the add component command would cause a panic. Thus solution was to add the DespawnMarker component and move actual despawning of any entity that could be modified to the Last Stage. This is basically a contrivance to get around the panic, and if we can solve it with our scheduling/ordering that is definitely preferable, although we should keep our eye on it to make sure it isn't reintroduced by having systems which can add components to entities and systems which can despawn the same entities within the same stage.

Removed Stages

I am fine with simplifying stages, but also with the understanding that if we find the need for more granular stages in future we will add as needed. I do agree that its annoying to figure out where a stage is added though, especially if stages are added outside of main.rs.

I'll return with more feedback as I go over code.

[zicklag]

The DespawnMarker was introduced because of an issue that was occurring due to parallelism in system schedule + commands application.

Oh, yeah, I forgot to come back to this after discovering that command flushing happened between stages and not at the end of the frame. The original design makes a lot more sense after realizing that.

If you want I can add it back, or we could wait until we have another problem, because the re-organization of the stages might have fix any problems from earlier.

but also with the understanding that if we find the need for more granular stages in future we will add as needed.

Absolutely. I just didn't want to keep extra stages around if they weren't helping yet.

I'll return with more feedback as I go over code.

👍 👍

[odecay]

Massive undertaking, nice job. Overall looking good, left comments.

[zicklag]

Great review!

I left responses where applicable and I'll push an update once I get the chance.

[zicklag]

OK, pushed a few updates resolving all but the last two unresolved conversations.

[zicklag]

bors r+

[bors]

Build succeeded:

• Check Rust formatting
• Clippy correctness checks (wasm32-unknown-unknown, web-target)
• Clippy correctness checks (x86_64-unknown-linux-gnu, target)

[edgarssilva]

@zicklag I really like the description on the wiki for the state machine. Are you ever considering creating a crate with it?

[zicklag]

Are you ever considering creating a crate with it?

The thought hadn't actually crossed my mind. 🤔 Interestingly, the non-game-specific code only adds up to 62 lines of code!

Still, it does seem like it could be useful as a crate, so that might be worth doing.