Add initial stat support for action counts #11
Conversation
NickCondron
changed the title
|
The action counts have reached or surpassed feature-parity with slippi-js action counts. Only exception is that slippi-js has logic to differentiate tech rolls based on player position. I skipped that part because it's only valid for 2-player games. 2-player specific stats can probably go somewhere else. I enforce a v0.2.0 version medium because accurate action counts are impossible without Some tests were 'borrowed' from slippi-js, but I don't feel bad because I wrote a few of them. Could probably use more tests for wavedash/dashdance Chose to make each action count |
NickCondron
force-pushed
the
action-counts
branch
from
4b26a9c
to
703a157
Compare
peppi/src/stats/actions.rs
Outdated
| } | ||
|
|
||
| fn is_ftilt(state: Common) -> bool { | ||
| state.0 >= Common::ATTACK_S_3_HI.0 && state.0 <= Common::ATTACK_S_3_LW.0 |
There was a problem hiding this comment.
I see why you wanted Ord on action states now. But I wonder what impact this has on the generated code for the surrounding match statement; it wouldn't suprise me if the compiler can't see through this to generate the same jump table it would if you wrote Common::ATTACK_S_3_HI | Common::ATTACK_S_3_HI_S | ... inline.
There was a problem hiding this comment.
Yea maybe not. However, I'm not going to worry about efficiency right now. Based on my tests, parsing is much slower than any reasonable stat computation we would do.
There was a problem hiding this comment.
What's your outlook for backwards compatibility of this module? It would be unfortunate if we had to bump peppi's major version number frequently just for stats. If that's a concern, we could potentially put stats in a separate crate.
Not all changes are breaking. For example:
- Adding new action type to
ActionComputer(ifActionStatisnon_exhaustive) -- non-breaking - Adding new
StatComputer-- non-breaking - fix stat to be more accurate -- non_breaking
I see your concern though. Stats doesn't need any crate-internal access, so it could easily be it's own crate. I don't know how that fits with your goals for peppi to be shared among different languages.
NickCondron
force-pushed
the
action-counts
branch
from
9b0b0de
to
5763492
Compare
| const MIN_VERSION: Version = Version(0, 1, 0); | ||
| fn check_version(ver: Version) -> StatResult<()> { | ||
| if ver < Self::MIN_VERSION { | ||
| Err(StatError::OldVersion(ver, Self::MIN_VERSION)) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Are there any pre-0.1 replays out there? Even if there are, IMO it's not worth spending any code on them at all.
There was a problem hiding this comment.
0.1 is the first replay version. This line exists so that you don't have to define MIN_VERSION if you want your stats to be available for all replays.
| @@ -52,6 +52,8 @@ pub mod model { | |||
| } | |||
| } | |||
|
|
|||
| pub mod stats; | |||
There was a problem hiding this comment.
Could you put examples of how to use this mod in the README? Both partial and one-shot methods, if I'm understanding the design correctly.
|
Closing because I plan on rewriting this once I figure out a better interface for it. I'm happy with the nuts and bolts of the implementation, but currently the types are clunky. I like your suggestion of making stats be its own crate. |
This is a draft becauseI'd like your input on how best to setup an interface for computing game stats like slippi-js does. Initial stats we will compute will be action counts because I implemented it in slippi-js, and I know how it works.Currently my main issue is that theEDIT: I figured out this partFrame<N>type is clumsy. Seems like we either bite the bullet and implement a conversion to some sort ofVecor we implement a macro that makes working withFrame<N>more reasonable. However, I've never used const generics before; maybe there's a better way.I want the interface to be idiomatic and not just copy what slippi-js does. I need help with the design. Implementing the calculations is more straightforward.