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

Record some stage hazards in new events #125

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

NickCondron
Copy link
Contributor

@NickCondron NickCondron commented Sep 22, 2023

Draft PR as I plan to add something similar for DL's wispy and PS's transforms. Let me know if this looks like a decent idea.

I finished adding events for the competitive stages.

@JLaferri
Copy link
Member

Keep in mind that you'll want to increase the possible total length of the transfer buffer. The symbol for that is FULL_FRAME_DATA_BUF_LENGTH in Recording.s

As for the changes themselves... I'm a bit torn. I don't know whether it's worth increasing the file size for this or not. What do you want to use it for? A preview display like slippilab.com?

@NickCondron
Copy link
Contributor Author

NickCondron commented Sep 22, 2023

I'm looking into doing some auto match analysis (like auto coaching). I got the motivation to work on it when I saw it was an issue on slippilab. It would also be useful for AI to understand where the platforms are.

The affect on file size is very small. It only affects 1/6 stages, only runs on frames where the platform changes (conservative estimate 1/20 of the time), doesn't run for each fighter, and each event is only 6 bytes. 1 byte in post frame update would have a way larger affect on file size. Wispy info would be even smaller.

Edit: Also is file size a major limitation right now? Disk space is really cheap, and replays can be compressed 10-20x easily

@JLaferri
Copy link
Member

JLaferri commented Sep 22, 2023

I'm looking into doing some auto match analysis (like auto coaching). I got the motivation to work on it when I saw it was an issue on slippilab. It would also be useful for AI to understand where the platforms are.

The affect on file size is very small. It only affects 1/6 stages, only runs on frames where the platform changes (conservative estimate 1/20 of the time), doesn't run for each fighter, and each event is only 6 bytes. 1 byte in post frame update would have a way larger affect on file size. Wispy info would be even smaller.

Edit: Also is file size a major limitation right now? Disk space is really cheap, and replays can be compressed 10-20x easily

Good to know about the frequency, that helps ease my concerns. File size is somewhat of a concern but small amounts won't impact much. Some people start to fill out all the free space in their discs, larger files are just a bit more frustrating for users.

Do you know which messages it gets sent between? Also I noticed that you didn't include the frame index in the payload, are you expecting to gather the frame index from messaging order? While I suppose that would work it's not necessarily guaranteed by the spec. Though given we have a frame start and frame end message now, maybe it should be?

@NickCondron
Copy link
Contributor Author

I added the frame index. I think it makes sense to have it. Currently it runs between pre and post frame updates.

However... It also runs once for each platform initially after gecko codes and before the first frame. I expected this might happen, but I hoped the FN_ShouldRecord call would cause them to be skipped. I'd prefer if those events weren't there because the initial height is constant, and the frame number is garbage at that point in time. I could probably do a hack where I check the link register to differentiate the call site (the first two are different). Maybe you have a better idea.

@JLaferri
Copy link
Member

I added the frame index. I think it makes sense to have it. Currently it runs between pre and post frame updates.

However... It also runs once for each platform initially after gecko codes and before the first frame. I expected this might happen, but I hoped the FN_ShouldRecord call would cause them to be skipped. I'd prefer if those events weren't there because the initial height is constant, and the frame number is garbage at that point in time. I could probably do a hack where I check the link register to differentiate the call site (the first two are different). Maybe you have a better idea.

Maybe you can check the scene frame?

@NickCondron NickCondron changed the title Record FoD platform heights in new event Record some stage hazards in new events Sep 25, 2023
@NickCondron
Copy link
Contributor Author

I added a new code to record whispy's blowing direction. I statically store the previous direction and avoid sending an event unless it changes. This event also runs between pre and post frame updates.

Comment on lines +16 to +20
# We skip to avoid the two initialization calls at game start
mflr r0
load r3, 0x801cc908
xor. r0, r0, r3
beq Skip
Copy link
Member

@JLaferri JLaferri Sep 25, 2023

Choose a reason for hiding this comment

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

Suggested change
# We skip to avoid the two initialization calls at game start
mflr r0
load r3, 0x801cc908
xor. r0, r0, r3
beq Skip
# We skip to avoid the two initialization calls at game start
loadGlobalFrame r3
cmpwi r3, 1
ble Skip

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer this over checking the LR, checking the LR feels quite hacky compared to checking the scene frame.

Copy link
Contributor Author

@NickCondron NickCondron Sep 25, 2023

Choose a reason for hiding this comment

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

Just tried this, but it doesn't work. The scene frame value is 0x000000d8 for the initial two calls. Maybe I could skip on that value, but can a platform try to move that soon?

Copy link
Member

Choose a reason for hiding this comment

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

Ah you're right I was testing with a replay and the behavior is different. It looks like the function gets called in the previous scene. So 0xd8 was just the frame on the SSS that you selected FoD. We might be able to check if we're in an in-game scene instead... lemme check some stuff.

Copy link
Member

@JLaferri JLaferri Sep 25, 2023

Choose a reason for hiding this comment

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

Did you confirm that FN_ShouldRecord did not work? I feel like it might.

EDIT: Yeah it looks like the scene has changed but the scene frame has not reset, that's kinda wild

Copy link
Member

Choose a reason for hiding this comment

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

So I'm thinking now that maybe the best plan is to add the following to SendGameInfo.asm

  li  r3,CONST_FirstFrameIdx
  stw r3,frameIndex(r13)

That way you know that the value has been initialized to the first frame and you can just use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it really much safer to modify the scene frame counter? I agree checking the LR is hacky, but I can't think of a way it would break.

Copy link
Member

Choose a reason for hiding this comment

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

Super late to this but that frameIndex is not the scene frame counter. That's Slippi's frame counter that starts at -123. Overwriting it there should be completely safe, nothing else in the game uses that.

Copy link
Contributor Author

@NickCondron NickCondron Feb 17, 2024

Choose a reason for hiding this comment

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

I did it this way and it worked.

@NickCondron
Copy link
Contributor Author

NickCondron commented Sep 27, 2023

I added a final event to send stadium transformation events. Here's an example plot of the ID values over the course of a replay:

output_plot

  • Ignore the blue it's a timer I decided not to use
  • Red is basically the current transformation. 3=fire, 4=grass, 5=normal, 6=rock, 9=water.
  • Green is the basically the different subtransitions during the transformation. 0=none, 3=monitor showing next, 4=platforms receding, 5=new platforms rising, 2/6 occur for one frame each but idk their purpose.

Right now the code sends red+green whenever either one changes. If we sent only red, you could logically reconstruct the green. However, it involves knowing and adding multiple different frame constants.

@NickCondron NickCondron marked this pull request as ready for review September 27, 2023 04:15
@NickCondron
Copy link
Contributor Author

@AlexanderHarrison
Copy link

What would it take to push this through? This is required for vod reconstructors. Willing to do some work if necessary.

@NickCondron
Copy link
Contributor Author

What would it take to push this through? This is required for vod reconstructors. Willing to do some work if necessary.

This PR is pretty much done, but there are a few considerations:

  1. The recorded FoD platform "heights" are the JObj y-offsets of the platforms when they change. This isn't exactly the same as the height of the platform, but there is a linear relationship. I just haven't worked out what the exact math is for that yet. You could figure it out pretty easily by examining a replay with a player on the platform as it moves. I'd like to have that documented before this is merged.

  2. I'd like feedback on the PS event values. See the graph above. Would the values there make it easy/simple to the stuff your interested in doing?

  3. The FoD gecko code checks the link register, so it's kinda hacky. I tried a couple alternative methods Fizzi suggested, but neither worked. I'm open to other suggestions, but I'm also tired of 'fixing' something that already works.

  4. It would make sense to have working slippi-js parsing code prepared for this branch. I have some rudimentary parsing code I've used for testing. I haven't gotten around to finalizing that.

Thank you for your interest.

@AlexanderHarrison
Copy link

AlexanderHarrison commented Mar 22, 2024

I've implemented FOD platforms in my reconstructor using this, and so far it looks good to me.

  1. On my end it seems like the recorded value is the raw platform height rather than the JObj y-offset.
    Given that character positions are recorded in that same coordinate system, I think the scale factor should already be commonly known (although I don't know it offhand). I imagine someone should know the exact factor already.

  2. I'll implement stadium transformations asap, although it is more difficult than FOD platforms for whatever reason (Individual transformation models are stored strangely). I'll give feedback after, but right now I don't see anything blatantly wrong or missing with this PS representation.

  3. Unfortunately I don't have any ideas about how to fix this - as soon as stuff gets put into RAM I'm out of my depth. If it allows an alternate, less hacky method, just recording the target position of a FOD platform and the frame it starts to move could be enough to recreate the platform movement on my end. Would also save a few kb per slp file.

  4. I can do this after we finalize the representations.

@AlexanderHarrison
Copy link

Implemented transformations. The PS transformation representation looks good to me.

I didn't end up using the subaction ids. You need to match over current_frame - ps_transform_start_frame anyway to determine things like the y-scale of the transformation model, and those didn't really line up with the subaction events, so in the end it was cleaner to not bother with subaction ids. That's just a preference, so I think it makes sense to keep them anyway. They could be useful for other purposes than vod reconstruction as well.

It would be nice to record the y-scale of the current transformation when it changes. It wasn't super difficult to reverse engineer the y-scaling, but recording it would make mine and other's lives a little easier. The easiest to use representation would only store the frame, the transformation id, and the y-scale. But this loses information when compared the existing representation and would require an implementation overhaul, so I think the current representation is fine.

Copy link

@PatrickHarden PatrickHarden left a comment

Choose a reason for hiding this comment

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

No idea what I'm looking at, but carpe diem

@AlexanderHarrison
Copy link

AlexanderHarrison commented Oct 15, 2024

Any thoughts on how to proceed here? Should we rethink the implementation? Should we record more hazards?

Just looking for some clarification on why this can't be merged in. This is very big for vod reconstructors.

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.

4 participants