vinceau
commented
1 year ago @JLaferri are you by any chance interested in going over this conversion logic change?
Open Walnut356 opened 1 year ago
vinceau
commented
1 year ago @JLaferri are you by any chance interested in going over this conversion logic change?
Walnut356
commented
1 year ago (I apologize for the essay, part of it is leaving breadcrumbs for myself)
Below is a link to a video of g9 top 8 replays parsed by my py-slippi fork (which uses the same logic as this PR) and clippi. I would have used clippi for both but I don't think have the tools/knowledge to build clippi from source without breaking something. Settings were at least 5 hits, at least 60%, must kill (fairly common use-case). I turned all of clippi's bonus features off (chain grab detection, single-hit damage threshold, etc.) since I don't have logic for that in py-slippi yet. They're matched to the frame, so if it only appears on one side, it was captured by one parser and not the other. There'll be some differences in the start/end padding as I wasn't able to match clippi's behavior exactly.
Most of the combo capture is identical. There are a handful that py-slippi didn't catch, and that looks like it's from a mix of sheik's up B and being airborne above the stage. I think the Y value check is definitely a good idea, but the threshold might require fiddling. 4:35 is a solid false-negative for py-slippi that I need to look more into. The only time I think the combo would have broken is the side B stall -> up B. The shield grab looks a little suspect, but everything that happened afterwards does more than 60% so if there were no other breaks it would have just started there. During the side-B it looks like his X coordinate is beyond the stage, so if I had to guess it's that the fall + drift inwards + up B startup meant he was under the stage
Py-slippi caught several that weren't caught by slippi-js, and those mainly had to do with holding shield. 5:07 in particular I'm not super happy with, but it's a judgement call. The WDOoS -> dtilt -> shield took 43 frames so it just barely didn't break. Flashing shield in quick succession can lead to unexpected behavior, but it's not 100% wrong either - he's still in a severe disadvantage state from the prior conversion. I've considered increasing the complexity of the logic a tiny bit for instances like these, but I'm not sure how y'all feel about adding more "visible" complexity. For example, in this case I'd make shielding stall the timer and maybe take up to 20 frames off, or reverse the timer by 1-2 frames for every frame it's active, rather than resetting it completely to 0.
There wasn't any that I'd consider true false-positives from either parser, but I'll try to find some sample replays that have some. I haven't watched top 8 yet so I can't comment on false-negatives that affect both parsers at the moment, but if you noticed some let me know and I'll see what I can do about fixing them.
As an aside, I noticed at 3:08 that the extra padding on clippi resulted in catching some startup shield pressure that was missed in py-slippi. In the future I could make that more intentional and less accidental by looking for hitlag while in shield that occurs within X frames of a conversion starting, then just swapping the start time of the conversion with the shield pressure's start time. That might also be useful for the extending "opening types" stat generation.
Letting ledge stalling extend the timer is more or less the same behavior as slippi-js. The current isInControl() only starts the timer with following action states (and their loop/start/end variants): wait, walk, turn, dash, run, jumpsquat, squat, [grounded A attacks]. That means all ledge actions and typical DJ/upB stalls are fair game, as well as haxdashes since land_fall_special won't start the timer. Shielding, universal (and aerial attack) land lag, and platform passthrough are some other "exploitable" absences. It's worth noting that it also only checks post frame action state, so if you can input something out of lag frame perfectly, you can "trick" the timer logic (e.g. land -> pre-frame wait + digital R press -> post frame shield looks like land -> shield to the conversion logic).
Jiggs and peach should be the only ones who can realistically exploit the offstage check via "on again off again" without getting hit. I don't think it's the worst to include those either, since being that close to the ledge is still a "severe(ish) disadvantage". Imo if you got there as a result of a conversion and then get hit out of it, it's not too different from convert -> shield pressure -> shield poke. If it becomes a problem, a "cooldown" can be added that just prevents it from toggling on for few seconds after it toggles off.
Walnut356
commented
1 year ago Here is a modified version of the previous logic compared to slippi-js:
The changes implemented:
Y value check (set to ~top platform). Requires isAirbone to be either true to return true, if isAirborne is null, it treats it as "true" for backwards compatibility with pre-2.0.0 replays.
Y value guard clause added to offstage check (y <= -5 and it returns true regardless of X value)
timer resets on fall_special (including drift variants)
timer resets on the first frame of land_fall_special if it wasn't preceded by knee_bend, escape_air, or the controlled_jump range*. This is specifically to account for recoveries like Sheik's that have a unique action state for their recovery endlag before entering fall_special.**
All slippi-js combos were detected, in addition to 8 more non-false positive ones that (at least in my opinion) are all totally valid combos. Genesis 9 top 8 isn't perfectly indicative of the total benefit though because of the character spread. Slower or less grabby characters like falco, peach, and many low tiers frequently have to settle with imperfect hit -> tech chase -> pressure the getup option. Slippi-js drops these because of the wait -> escape option/shield issue. Those characters will be the ones that'll likely have the most dramatic change in parsing results.
*I forgot to add land_fall_special itself to this list, so wavedashes/wavelands resulted in 2 false-positives at 5:34 (combo dropped at 6:13 in-game time, takes 3 hits) and 7:45 (combo dropped at 6:49 when fox gets grabbed). I manually rechecked those 2 with that fixed and... well it's a bit subjective. The first false-positive is gone entirely, the second had its start time moved forward to frame 4413 (first shine after sheik fails tech chase, remaining combo still passes damage check by literally 1%). It also removes the combo at ~10:28 video-time. It feels like a combo, but upon closer inspection it's totally reasonable to agree with the parser - it was dropped at ~7:48 in-game and re-picked up at ~7:45, with the latter half of the combo doing only 38%.
**If the above fix isn't enough to handle laggy recoveries properly (e.g. if they go a mid height, and the travel time + special animation/fall distance >= 45) i have an extra club in my cave, which is just a dictionary of {character : [recovery move endlag action state id's]}. Some side B's are counted (e.g. fox/falco), jigg's rest state can slip in there because it's convenient (and if it kills it'll break the combo anyway, so it'll only extend for missed rests), but pseudo-recovery moves that are used somewhat frequently on stage (e.g. marth sideB 1, yoshi up b) aren't included. I haven't added it so far because it'd take a lot more testing. B-move action states are a non-uniform disaster and there's so much behavior I just can't predict. Not every move has a unique ID for just the endlag, not every move puts you in land_fall_special if you land during the endlag (and it has no relation to having a unique endlag IDs afaik, see: falco side b vs shiek upB), some are commonly used onstage and don't have unique endlag ID's, and if you land before entering fall_special you're put in a very punishable land/land_fall_special (samus upB has 23 frames of land). Does including that make for more false positives than it reduces false negatives? Who knows. If y'all are happy with the current output I'm not even gonna touch it because it's just spaghetti.
JLaferri
commented
1 year ago So first off, thanks for making these videos. They really help visualize the difference.
I think that after watching them, however, I don't see much reason to switch the logic.
The idea behind the conversions is to try to detect a "reset to neutral" in order to stop the combo. In many of the "new" occurrences following your modification, in my opinion many of the clips have a more visible reset to neutral with the same player winning again or the player being punished messing up.
I still believe the original logic is quite logical. There are likely still some issues, but I'm not convinced this new method is better. Some of the problems as I see them are:
Overall I think the concept of "player re-establishes control of their character on stage and doesn't get hit again for a certain amount of time" is a perfectly fine concept for determining if neutral is re-established.
Walnut356
commented
1 year ago Top 8 at a super major maybe isn't the most representative because the overall quality is so high. For most players, it results in a lower number of total detections with a higher average quality. For example, this is a completely valid conversion with slippi-js's logic. The py-slippi logic properly cuts it at several points in the middle, so it doesn't make it past the filter. False positives like these happen in mid and low level replays constantly and it makes the conversion calculator a chore to use for personal/local combo videos. It nearly always requires an additional pass from the start to weed out the inevitable pile of "this isn't even a conversion" clips that slip through.
Fundamentaly, the py-slippi logic still adheres to the idea of detecting "resets to neutral". The difference is that I tried to go about it from the opposite perspective - "what states will the opponent be in if they're being combo'd?" vs the current "what states must the opponent enter on their way out of a combo?".
Imo it makes for more readable and modifiable code, as the game's representation of the former is much more consistant (no fiddling with unique action state IDs, pre-vs-post frame state updates, etc). Modifying the existing logic to keep the isInControl() check while fixing the loopholes requires adding a bunch more stuff to isInControl() itself (land, probably aerial land lag, B move action state spaghetti, probably some character specific stuff like float/jiggs jumps remaining, maybe even more), and then in the conversion calculator you'd probably need to add a nested conditional to the opntIsInControl to fix the buffered shield/roll issue. Representing and fine-tuning the py-slippi logic is a lot more straight forward.
You're 100% right though that "resetting to neutral" isn't the same character to character or matchup to matchup. I fiddled with the idea for a while, but I don't know that it's really possible to represent that accurately. At the very least, py-slippi logic should handle your jiggs example more accurately than slippi-js. It only sees airborne targets as "in a combo" if they're at top platform height or above (threshold could be adjusted up or down easily) - a jiggs DJing above the ground without getting hit for 45 frames would be cut. The assumption is that if someone's above top platform height, their options are much more limited and there's a much higher chance that they're in a bad situation. If they're below that, it's reasonable that they'll be hit within 45 frames, and if they aren't they've probably "escaped".
The crux of it is the idea of "severe disadvantage". Even if you're not actively getting hit and you have control of your character, that doesn't necessarily mean you're back to neutral. "That movement is illegal ice" is a perfect example of that imo. At no point after the dthrow are they in neutral even though Aramda has the time and space to throw out options. Nearly every option is bad and anything he goes for (even trying to escape) is basically a guess/read. If you trace back from the killing usmash, the only time Armada actually has all options available to him (without having to make a panic guess the moment he has control) is before the dthrow. He could have ended up in neutral if he had made different decisions, he could have won the scrap, etc. but I don't think that invalidates the conversion any more than "you could have DI'd to escape" invalidates any other combo.
There's no perfect way to represent it, but py-slippi will at least catch some scenarios with abstract bait-y pressure when slippi-js does not.
As a warning, I don't really know JS/TS so some of the formatting/type hints/non null assertions might be wrong.
Basically, conversion timer reset logic is based off of checking for a handful of actionable states that indicate that someone has escaped a combo. That check, per its comment, isn't complete. As a result there's a lot of weird edge cases and "false positives" that let conversions go on when they shouldn't. Additionally, with the weird way that the reset counter was set up, conversions would end before they should in some instances (e.g. mid-combo shield pressure that lasts more than 45 frames, shield breaks with long startup moves, etc.).
It's WAY easier to check for the usual non-actionable states than the actionable ones. I've been using this modified logic (in addition to checking the player bitfields for hitstun and hitlag) in my fork of the py-slippi parser and it's been working quite well.
I added in helper functions in the same format as the existing ones, and then added those checks to the conversion calculator, and matched the reset functionality to the one in combo.ts. One of the checks requires checking the stage, so that value is now passed to handleConversionCompute. Additional checks could be added to refine the "offstage" x value check (check Y to make sure they're not underneath battlefield, or to see if they're high enough that they're likely being juggled), but those situations are niche, require some fine-tuning, and come with some false positives, so I've left them out for now. It also might be worth adding the jigg's sing sleep state and jigg's rest state but I wasn't 100% sure what the IDs for those were.