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

Parse stage events #135

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

Conversation

AlexanderHarrison
Copy link

@AlexanderHarrison AlexanderHarrison commented Mar 24, 2024

For this pr recording stage events.

@AlexanderHarrison AlexanderHarrison marked this pull request as ready for review January 16, 2025 00:26
src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@vinceau vinceau left a comment

Choose a reason for hiding this comment

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

I'm not really familiar with the changes in the ASM repo but I would have thought maybe a single STAGE_TRANSFORMATION_EVENT or something that can be used for all the different stages might be better. Not sure how that would be done, but I just fear that the Command Enum will essentially balloon like crazy if each possible stage event has its own unique Command.

Not sure if @JLaferri wants to chime in.

slp/FODPlatforms.slp Outdated Show resolved Hide resolved
@AlexanderHarrison
Copy link
Author

Paging @NickCondron, the implementer of stage events.

Each stage event has its own command byte, and the Command enum mirrors that like every other event. Although it is technically possible for the stage events to be under a single command byte, it drastically simplifies the implementation and parsing to separate them. And I don't think we are in any danger of running out of command bytes.

@NickCondron
Copy link
Contributor

I'm not really familiar with the changes in the ASM repo but I would have thought maybe a single STAGE_TRANSFORMATION_EVENT or something that can be used for all the different stages might be better.

Each stage has unique semantic information. You could unify them into a single STAGE_TRANSFORMATION_EVENT, but then you'd need an interior discriminant/subtype field which kinda brings you back to square one. They are unique commands in the ASM because they each have their own injection point and it makes the ASM simpler to do it that way.

The implementation in this PR looks reasonable to me at first glance. I would only add to make sure that the code handles the case where both FoD platforms are moving at the same time because you will get two events per frame in that case.

Not sure how that would be done, but I just fear that the Command Enum will essentially balloon like crazy if each possible stage event has its own unique Command.

Personally, I don't foresee other stages/hazards being added. Yoshi's Story has Shy Guys, but they get captured in the items event and Randall is predictable based on the timer. FD and BF don't have hazards. The number of people with the ability and desire to do this for non-competitive stages is approximately zero lol.

@JLaferri
Copy link
Member

Yeah the separate commands is how it was done in the ASM and I'm cool with it. I don't really expect many (any?) more to be added and it allows for the stage events to each have the correct size, making the final SLP smaller in some cases.

Copy link
Member

@vinceau vinceau left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the clarifications, making those changes, and for adding tests too! Happy for this to be merged when the ASM/Dolphin changes are in :)

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