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

Load attack properties from assets #221

Merged
merged 4 commits into from
Aug 11, 2022
Merged

Load attack properties from assets #221

merged 4 commits into from
Aug 11, 2022

Conversation

odecay
Copy link
Collaborator

@odecay odecay commented Aug 10, 2022

This loads basic attack properties from YAML fighter definition files.
AttackFrames, hitbox size and offset relative to fighter are currently loaded.
I've also added a stationary Punching attack, used by enemies AI and player. Flop attack still exists but is currently disabled with the intent to re enable after input buffer + attack chain system work.

Fighters now have attack frames which better fit the differing lengths of their attack animations, though the placement and sizing of hitboxes are currently not tuned to match the sprites, though easily modifiable via fighter definitions if need arises.

Direct follow up work to this is additional attack properties loaded from YAML, but this should be left till after other basic attack work is in place.

  • Knockback(whether/how much it moves the character hit)
  • Hitstun(how long the enemy takes to recover and be able to perform actions when hit),
  • Movement (how much it moves the character performing the attack),
  • Invincibility/hyperarmor (whether the attack should be able to be interrupted).

Copy link
Member

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

Did a quick review of the code and looks good. Left a few comments on your comments.

assets/fighters/bandit/bandit.fighter.yaml Outdated Show resolved Hide resolved
src/attack.rs Outdated Show resolved Hide resolved
@@ -535,6 +579,7 @@ fn attacking(

// Do a forward jump thing
//TODO: Fix hacky way to get a forward jump
//should moving and any attack that requires movement be a non exclusive state?
Copy link
Member

Choose a reason for hiding this comment

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

Not 100% sure I understand what this means, but my gut reaction is that movement made during an attack should be manged by an exclusive attack state most of the time. Unless you can literally walk while attacking without the attack interfering with the walking movement at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment should have read "additive state" but I honestly don't really remember what I was getting at with this one either.

Copy link
Member

Choose a reason for hiding this comment

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

non-exclusive sounds nice too. 🤔 I could almost go with either terminology. :)

commands.entity(entity).insert(fx_playback);
}
} else {
//what should we actually do if we cant read attack metadata from fighter?
Copy link
Member

Choose a reason for hiding this comment

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

Technically this shouldn't happen because we have logic to make sure all assets are loaded before starting gameplay. We could maybe panic with a message saying that much so that we know if we accidentally break pre-loading assets maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is the correct way to query the metadata then? Shouldn't have to worry about the handle not returning something?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. The handle should always return or we've done something wrong in the loader.

So we could either .expect("Handle not loaded") or something like that, or we could warn!("Skipping attack due to un-loaded fighter meta").

I'm on the fence between the two. If we panic, we won't accidentally mis the log message, but it might crash the game for a user who doesn't know what to do about it, and it might only cause a 1-frame problem anyway and it could have been safely ignored.

If we don't panic, we face the possibility of not seeing the warning during development, and being more likely to ship a release of the game with a bug in it.

Difficult trade-off to decide between. 🤷‍♂️

@zicklag
Copy link
Member

zicklag commented Aug 10, 2022

Design wise I'm starting to feel like we might want to have different attacks have different properties, but be applied in a limited number of attack states.

If we end up with scripts defining attacks, then it might make sense to have free-form attack metadata, and it would be the responsibility of the attack state handler system to parse the attack options applicable to that particular attack state.

That way each attack could be configured in whatever way made sense for that attack.

That's just a quick, thought, I haven't thought carefully about it yet.

@odecay
Copy link
Collaborator Author

odecay commented Aug 10, 2022

Design wise I'm starting to feel like we might want to have different attacks have different properties, but be applied in a limited number of attack states.

If we end up with scripts defining attacks, then it might make sense to have free-form attack metadata, and it would be the responsibility of the attack state handler system to parse the attack options applicable to that particular attack state.

That way each attack could be configured in whatever way made sense for that attack.

That's just a quick, thought, I haven't thought carefully about it yet.

The properties I'm thinking of would be things like:

  • Knockback(whether/how much it moves the character hit)
  • Hitstun(how long the enemy takes to recover and be able to perform actions when hit),
  • Movement (how much it moves the character performing the attack),
  • Invincibility/hyperarmor (whether the attack should be able to be interrupted).

all those seem like they would be pretty easy to represent in asset files other than movement

@zicklag
Copy link
Member

zicklag commented Aug 10, 2022

Oh, I see that makes sense. Those are attributes as far as what the attack does to the receiver of the damage. I was thinking of attributes effecting how the attack is performed.

Yeah, those attributes make a lot of sense to me.

@odecay
Copy link
Collaborator Author

odecay commented Aug 10, 2022

Oh, I see that makes sense. Those are attributes as far as what the attack does to the receiver of the damage. I was thinking of attributes effecting how the attack is performed.

Yeah, those attributes make a lot of sense to me.

Any examples/ideas of the sorts of things you are thinking of?

@zicklag
Copy link
Member

zicklag commented Aug 10, 2022

I just meant like if we have different kind of attacks like a ground-pound. It could be defined in Rust or in a script, either way. Then when you add that attack to a fighter you'd want to be able to configure things about the ground pound, like how big the damage radius is, how much damage it does, etc.

But there could be any number of different kind of attacks, such as punch, flop, etc. And those wouldn't have damage_radius they would just have damage, so we might need to allow a flexible attack options field at some point.

That might be somewhat relevant really soon actually, because already we've got the stomp that the boss is going to be doing, and then we'll have the slingshot attack of the other fighters, but we don't have a great way yet to say which of these attacks the fighter will do when they press a certain button.

_I'm a little distracted at the moment so beware, not all of my explanations are 100% thought through right now. 🙃 _

@odecay
Copy link
Collaborator Author

odecay commented Aug 10, 2022

For sure those all make sense.
I was also thinking about both those cases as well. Soon each fighter will have multiple attack types as well.
Its hard to say exactly where the line is for which attacks should be represented as their own state + transition systems and which should be loaded as properties. (I think thats what we are talking about here)
We will probably figure out what designs map best as we progress.

@odecay odecay marked this pull request as ready for review August 11, 2022 06:34
@odecay
Copy link
Collaborator Author

odecay commented Aug 11, 2022

This is all I intend to put in this PR for now.
Will probably have effect on #222

@edgarssilva edgarssilva reopened this Aug 11, 2022
@edgarssilva edgarssilva mentioned this pull request Aug 11, 2022
@edgarssilva
Copy link
Collaborator

Sorry about that closed the wrong PR.

Copy link
Member

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

Looks good. We'll just have to resolve conflicts. If you want, I can rebase this onto the changes from #222 that were just merged.

I think we can take out the boss flag, now that we have an attack name field which we can use to decide which attack the fighter triggers.

@odecay
Copy link
Collaborator Author

odecay commented Aug 11, 2022

oof rebase + rebase.
The boss AI is currently not functional but I really don't want to keep rebasing this so gonna put it in and address that in follow up.

@odecay
Copy link
Collaborator Author

odecay commented Aug 11, 2022

bors merge

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