search

ArchipelagoMW / Archipelago

Archipelago Multi-Game Randomizer and Server
https://archipelago.gg
Other
480 stars 636 forks source link

Yoshi's Island: Fix dumb client bug #3586

Closed PinkSwitch closed 3 months ago

PinkSwitch commented 3 months ago

What is this fixing or adding?

Fixes(?) A bug in the Yoshi's Island client which causes Goal completion to be registered when it is not intended to; Should fix a bug of clients accidentally goaling early when connecting improperly or when attempting to connect via FXPack

How was this tested?

It wasn't, since I am unable to reproduce the bug at all, but I tested that connection and goal still works.

If this makes graphical changes, please attach screenshots.

N/A

mrkssr commented 3 months ago

Can't say anything to the code changes but to the SuperNT/FxPak Pro behaviour.

I've generated a game with the 0.5.0 RC3 installation and one with this branch. The 0.5.0 RC3 generated game released everything immediately after starting the game (not even a save file, almost after just starting the game itself). The changes in this branch don't do that anymore. So it seems to work for that scenario.

I guess FxPak Pro is still not supported. I cleared a level but no item were sent. It seems not to be the goal of this PR anyway.

PinkSwitch commented 3 months ago

Can't say anything to the code changes but to the SuperNT/FxPak Pro behaviour.

I've generated a game with the 0.5.0 RC3 installation and one with this branch. The 0.5.0 RC3 generated game released everything immediately after starting the game (not even a save file, almost after just starting the game itself). The changes in this branch don't do that anymore. So it seems to work for that scenario.

I guess FxPak Pro is still not supported. I cleared a level but no item were sent. It seems not to be the goal of this PR anyway.

This is specifically fixing unintended client behavior, there was no intent to fix FX pack?

PinkSwitch commented 3 months ago

This does look like a dangerous check, only checking 1 byte, and only checking that it's not zero. I would try to look for something more specific.

But this looks like it won't make it worse.

This does look like a dangerous check, only checking 1 byte, and only checking that it's not zero. I would try to look for something more specific.

But this looks like it won't make it worse.

It's been mostly fine; the byte is only written to during the ending sequence; the problem was that it was being checked before all of the rom validation checks. The fix pushes it back so it's only read when it's intended to be.