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

[Bugfix] Add check for animation count to prevent continuing final saw textbox while putaway is happening #3103

Merged
merged 3 commits into from
Aug 6, 2023

Conversation

Malkierian
Copy link
Contributor

@Malkierian Malkierian commented Aug 3, 2023

Fixes #2200 - softlock when holding an item and skipping text giving the saw.

Build Artifacts

@Malkierian Malkierian mentioned this pull request Aug 3, 2023
@Archez
Copy link
Contributor

Archez commented Aug 4, 2023

Nice find! The solution is rather simple and self contained, I like it.

My only question is, with this technically being a authentic vanilla bug (that is exasperated due to Skip Text) and the fix is to alter the actor, should we consider wrapping this as a "fix toggle" in the menu? Maybe add it to the rando preset, or just apply the fix always in rando?

Given our stance on previous fixes and trying to preserve the authentic experience, I think this falls in that line. The only concern I guess is hoping people turn it on 😅

Edit:
Or maybe have a fix toggle, but also have so if Skip Text is on, the fix toggle is forced on?

@Onemario1234
Copy link

Onemario1234 commented Aug 4, 2023

Or maybe have a fix toggle, but also have so if Skip Text is on, the fix toggle is forced on?

That would be best.

My only question is, with this technically being a authentic vanilla bug (that is exasperated due to Skip Text) and the fix is to alter the actor, should we consider wrapping this as a "fix toggle" in the menu?

It wouldn't be the first fix on a level like this to get made into a toggle. Isn't there a vanilla bug that can lock you out of an upgrade if the fix isn't enabled?

@Malkierian
Copy link
Contributor Author

Technically, it wasn't a bug until skip text was introdced. You would never be able to encounter it in vanilla without that functionality, at least as far as I know. I'd consider this exclusively fixing a bug we introduced with our own feature.

@Malkierian
Copy link
Contributor Author

Although, I could see an argument for not enabling it unless skip text is also enabled. I just don't see a reason to make it its own toggle.

@briaguya-ai
Copy link
Contributor

I think just extending the comment in code to explain that this doesn't happen with authentic text speeds/no skip text, but is needed because of skip text, provides enough context for the deviation from authentic code for people to know it shouldn't change authentic behavior.

@Archez
Copy link
Contributor

Archez commented Aug 4, 2023

Technically, it wasn't a bug until skip text was introduced

that this doesn't happen with authentic text speeds/no skip text

I made my comment above because this is replicate-able on n64 hardware by simply mashing A/B after turning in the Poacher's saw. Others in the Discord have also commented that this is a vanilla issue with text mashing.

I just replicated it now via PJ64. And it can be replicated in SoH with skip text off & fast text off.

2023-08-04.08-53-39.mp4

Fixing this is fixing a authentic vanilla bug that you can experience without any enhancements, similar to the Deku Nut upgrade fix (which has a toggle).

@Malkierian
Copy link
Contributor Author

Interesting. Why were they so inconsistent with their textboxes? I guess I'll have to make a fix toggle for it, but I still think it should be automatically enabled with text skip.

…rdless of previous selection for the Fix toggle itself (so it works if you have the Fix toggle enabled, or if you have Skip Text enabled).
@Malkierian
Copy link
Contributor Author

Fix toggle has been added. Skip Text forces the behavior.

@briaguya-ai
Copy link
Contributor

I'd be fine with having it as an "unfix" box. Regardless of skip text default to fixing it, but leave an option to restore the authentic behavior

@Malkierian
Copy link
Contributor Author

There is no authentic behavior when combined with skip text, though. If you don't have skip text on, then it's just a normal checkbox. I can't see why we'd want to have it able to be "unfixed" with our own enhancement enabled. If you enable the enhancement, you're already not worried about getting the authentic behavior.

@briaguya-ai
Copy link
Contributor

I'm just saying if this is a bug people can encounter without skip text I'm in favor of having it fixed by default regardless of skip text

@Malkierian
Copy link
Contributor Author

Ohhhh, I gotcha. We'll see what others have to say on the idea.

@Archez
Copy link
Contributor

Archez commented Aug 5, 2023

We don't do that currently with similar bug fixes like the Deku Nut upgrade fix, which is arguably also something casual players can experience. I'm not completely against the fix being defaulted to on, but calling it an "unfix" feels weird to me, and it would be a precedence change.

We could just throw it in the presets which is what we already do for the Deku Nut upgrade fix.

@Malkierian
Copy link
Contributor Author

Yeah, the presets seems the best option. Pretty much anything not default I'd say.

@briaguya-ai
Copy link
Contributor

I know we've had back and forth around how to handle authentic bugs before (I'm mostly thinking of the convo on #1752)

I'd be happy to open the conversation around having bugs that default to fixed up again. I feel like providing the authentic experience (bugs and all) as an option is important, but doesn't need to be the default.

@Malkierian
Copy link
Contributor Author

I just realized it would be pointless to add the fix to the presets because of the way I force the behavior if Skip Text is on and Skip Text is in all the presets already XD

Copy link
Contributor

@leggettc18 leggettc18 left a comment

Choose a reason for hiding this comment

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

LGTM

@leggettc18 leggettc18 merged commit f68a4e9 into HarbourMasters:develop-sulu Aug 6, 2023
@Malkierian Malkierian deleted the saw-softlock branch August 6, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants