Gravedigging tour fix #388

leggettc18 · 2022-05-20T02:22:06Z

Basically just causes Dampe's Gravedigging Tour Heart Piece to set a Collect flag on the Graveyard Scene when collected instead of a GetItemInf flag when it's spawned. I did this by simply saving the result of Item_DropCollectible to a variable called reward and running reward->collectibleFlag = 0x19 if the reward was a heartpiece. As far as I can tell, the only other collectible in Spot02_Graveyard (the crate heart piece that you need the flying magic bean plant for) does not share this flag, so it should be safe to use.

There may be a better way to do this. This is unlike most of the other dropped items with collectible flags in the game, which have some binary operations performed on the item to be dropped before passing it into Item_DropCollectible. See z_en_geldb.c and z_bg_haka_tubo.c for examples of this. I tried to find some way to do something more like that here but I was unable to wrap my head around the binary operations being performed. I may revisit this in the future.

Basically just causes Dampe's Gravedigging Tour Heart Piece to set a Collect flag on the Graveyard Scene when collected instead of a GetItemInf flag when it's spawned. I did this by simply the result of Item_DropCollectible to a variable called reward and running reward->collectibleFlag = 0x19 if the reward was a heartpiece. There may be a better way to do this. This is unlike most of the other dropped items with collectible flags in the game, which have some binary operations performed on the item to be dropped before passing it into Item_DropCollectible. See z_en_geldb.c and z_bg_haka_tubo.c for examples of this. I tried to find some way to do something more like that here but I was unable to wrap my head around the binary operations being performed. I may revisit this in the future.

jbodner09 · 2022-05-20T08:15:07Z

Tooltip is long enough that it could use a newline or two to break it up

leggettc18 · 2022-05-20T11:51:44Z

Oh, for some reason I just didn't even think about newlines in the tooltips being a thing at all. I'll add one or two.

leggettc18 · 2022-05-20T12:20:58Z

Ok I added some new lines to that tooltip.

PurpleHato · 2022-05-20T12:26:25Z

I think
"Fixes a bug where you can permanently miss the Gravedigging Tour heart piece"
is enough as the tooltip, since it's basically the only way to miss the piece of heart, else Fixes will always be like super long and wide ^-^

leggettc18 · 2022-05-20T12:55:41Z

You're probably right, I didn't know about the bug before I started fixing it, so I felt the need to explain what the bug was, but interested parties can just Google it.

One thing I forgot to mention is that I currently set the old flag no matter whether the cvar is set or not, but the new flag only gets set if the cvar is on, and the cvar also determines which flag gets checked. The idea was that a player couldn't duplicate the heart piece by checking the box, getting the heart piece, and then unchecking the box, but I just realized the way it is currently still possible to duplicate it if you get it with the box unchecked and then check the box. I could fix this by also setting the new flag regardless of cvar value, but that feels a bit weird to do. Should I do that and just let the cvar determine which flag gets checked? Or should I give up on the anticheat mechanisms and just check and set one flag or the other?

EDIT: changed set to checked in the second to last sentence.

leggettc18 · 2022-05-20T15:06:59Z

I also just realized I accidentally left in the this->currentReward = 3 that I hard-coded for testing, so I wouldn't have to deal with the RNG to make sure the flag gets set correctly when the Heart Piece is picked up. I'll fix that tonight when I get off of work.

jbodner09 · 2022-05-20T16:28:13Z

I also just realized I accidentally left in the this->currentReward = 3

I almost said something about that since I thought that was weird! My view on anti-cheating is that, if it wasn't possible to dupe the heart pieces before your change, then it shouldn't be possible after your change, which means doing whatever you need to in order to enforce that.

leggettc18 · 2022-05-20T16:45:19Z

Ok, that's kinda what I was thinking, I was just hesitant since it requires setting a flag that wasn't set in vanilla whether the cvar introduced is set to on or off, which felt a bit weird to do. Although I suppose the check for that flag is the only thing that actually changes any behavior and that check would still be locked behind the cvar being nonzero.

So that's 3 changes for tonight then:

More concise tooltip
Remove the this->currentReward = 3
Set the reward->collectibleFlag = 0x19 regardless of cvar gGravediggingTourFix value to prevent Heart Piece duping.

If there's anything else I'd be happy to add it to the list!

leggettc18 · 2022-05-21T01:49:25Z

Ok, that should fix the above points.

leggettc18 · 2022-05-21T19:47:37Z

Resolved some merge conflicts resulting from the recent commits to the develop branch.

leggettc18 · 2022-05-21T19:53:12Z

If this were to get merged, should this be on by default, like the Deku Nut Upgrade fix is, or off by default?

InfoManiac742 · 2022-05-21T19:59:52Z

If this were to get merged, should this be on by default, like the Deku Nut Upgrade fix is, or off by default?

I don't see why it should be off, there's not really a case where you'd want to lock yourself out of a heart piece.

leggettc18 · 2022-05-21T20:02:57Z

OK, I guess I just wasn't sure if you all would be OK with the default experience differing from the vanilla N64 experience in that way.

MelonSpeedruns

LGTM

PurpleHato

LGTM

mzxrules · 2022-05-27T09:15:30Z

soh/src/overlays/actors/ovl_En_Tk/z_en_tk.c

+                // Initialized with bugfixed condition, overriden with original check if bugfix cvar is 0 (which is default).
+                bool condition = Flags_GetCollectible(globalCtx, 0x19) == 0;
+                if (CVar_GetS32("gGravediggingTourFix", 0) == 0) {
+                    condition = !(gSaveContext.itemGetInf[1] & 0x1000);
+                }
+                if (condition) {
                    this->currentReward = 4;
+                    gSaveContext.itemGetInf[1] |= 0x1000;
                }
            }


Suggested change

// Initialized with bugfixed condition, overriden with original check if bugfix cvar is 0 (which is default).

bool condition = Flags_GetCollectible(globalCtx, 0x19) == 0;

if (CVar_GetS32("gGravediggingTourFix", 0) == 0) {

condition = !(gSaveContext.itemGetInf[1] & 0x1000);

}

if (condition) {

this->currentReward = 4;

gSaveContext.itemGetInf[1] |= 0x1000;

}

}

if (!(gSaveContext.itemGetInf[1] & 0x1000)

|| (CVar_GetS32("gGravediggingTourFix", 0) != 0 && !Flags_GetCollectible(globalCtx, 0x19))) {

gSaveContext.itemGetInf[1] |= 0x1000;

this->currentReward = 4;

}

}

This should make the logic look a little cleaner. if the itemGetInf flag isn't set, it should always enter the if statement (assuming nothing else sets that flag), otherwise we check if the fix should be applied and that the collectible flag hasn't been set yet. Not familiar with how CVar_GetS32 is used, but I imagine the != 0 isn't necessary unless it's needed for style reasons.

You are correct that the != 0 probably isn't required. I didn't feel like experimenting with whether or not C automatically saw zero as false and nonzero as true so I just went explicit with the boolean expression. I have since double checked that it does so I could change that.

As for the larger change here, wouldn't this make it so that if someone triggered the glitch with the box unchecked by getting the heart piece and leaving, they would then be unable to get the Heart Piece by checking the box and trying again? The old flag still gets set to preserve the vanilla behavior, and the OR clause in your change would then return true with that flag set whether the CVar for the fix is 1 or 0. My current code already covers this case, by not checking the vanilla flag if the fix is enabled, but if it would be better for readability I could take your check and add another && for the CVar being 0. So the full condition check would be

if (!(CVar_GetS32("gGravediggingTourFix", 0)) && !(gSaveContext.itemGetInf[1] & 0x1000) || CVar_GetS32("gGravediggingTourFix", 0) && !(Flags_GetCollectible(globalCtx, 0x19))) {

I have doubts if that is really more readable though. That's a pretty lengthy boolean expression and has multiple calls to CVar_GetS32("gGravediggingTourFix, 0) whereas my current code only has one.

As for the larger change here, wouldn't this make it so that if someone triggered the glitch with the box unchecked by getting the heart piece and leaving, they would then be unable to get the Heart Piece by checking the box and trying again?

it shouldn't. the simplified pseudocode here is this:

if (!itemGetInf) OR (fixEnabled AND !collectibleFlag) { set itemGetInf true set reward to heart container }

The optimization here is that if the itemGetInf flag is false, it is safe to assume that the collectibleFlag is also false regardless of whether the fix should be applied or not. This is because itemGetInf flips to true when the heart container is spawned for the first time, and if the heart container hasn't ever spawned, then collectibleFlag can't be true. The OR here simply introduces an extra alternative condition for spawning the heart container.

Toggling the option should not lead to duplicates or making the heart permanently skippable; this is mainly because with the code written this way, the itemGetInf state and collectibleFlag state is always set the same way, independent of the state of the fix flag.

Lastly, writing things this way has the additional side effect where if someone wanted to reset the heart piece, they would only have to zero the original flag, rather than two different flags depending on which setting was enabled.

OH I see, you're right, I think I misinterpreted the check on the old flag as it being collected returning true rather than not collected. That is a good point that I didn't see right away and the last point you made is also good.

…into gravedigging-tour-fix

This originally introduced a bug where the player could spawn multiple heart pieces by simply not collecting the one that spawns and continuing to dig up spots. This fixes that by checking a temp clear flag before spawning the heart piece and setting it when the heart piece spawns. Since this is a temp clear flag it will not stay set if the player exits the scene, so this still does fix the bug of locking the player out of the heart piece when spawning it and leaving without picking it up. As far as I can tell this temp clear flag isn't used anywhere else in this scene. The only one used in this scene I could find is that killing the first Poe in this scene sets flag 0x02 (or maybe it's 0x01, not sure if the flags start at 1 or 0).

…into gravedigging-tour-fix

leggettc18 · 2022-05-29T16:42:33Z

I implemented @mzxrules suggestion to simplify the check condition for whether or not to spawn the heart piece, and also added an additional check to prevent players from spawning multiple heart pieces by not collecting the piece and not leaving the scene, but continuing to play the Gravedigging Tour. See commit a4c7833 for details.

…into gravedigging-tour-fix

mzxrules · 2022-05-29T17:51:48Z

you can probably just create a new local variable in z_en_tk that stores whether the heart piece has been spawned, instead of reserving a second collectible flag.

PurpleHato · 2022-05-29T17:53:32Z

Tested the windows version of that fixes from https://builds.shipofharkinian.com/job/SoH_Multibranch/view/change-requests/job/PR-388/ that has been recently built with your recent changes and crashes instantly on Windows

leggettc18 · 2022-05-29T18:00:44Z

Yeah I'm seeing the crash too, I definitely had it working a few minutes ago though, trying to figure out which commit started crashing.

leggettc18 · 2022-05-29T18:10:57Z

As far as I can tell, something in the most recent commit to the develop branch causes my code to crash the game. Develop apparently works fine without this PR, and my PR works without the most recent commit from develop, but something about the combo of the two is causing the game to crash. Working on figuring out why now.

leggettc18 · 2022-05-29T18:15:53Z

you can probably just create a new local variable in z_en_tk that stores whether the heart piece has been spawned, instead of reserving a second collectible flag.

Well what I reserved was a TempClear flag rather than a collectible flag to be clear, but I guess the point still stands that a local variable might be better.

leggettc18 · 2022-05-29T20:09:32Z

Apparently Develop was crashing on Windows for several people, as we found out on the Discord channel, so turns out the crash was not caused by my code. The fix has been identified, just waiting for it to get put in.

…ke they made more sense there)

jbodner09 · 2022-05-29T22:05:20Z

Looks like that fix is in #425

SomeoneIsWorking · 2022-06-02T20:43:57Z

I didn't even think of having bug fixes togglable leading to problems when being switching them during gameplay and contributors also having to prevent those problems.
Thankfully this game is mostly stable but having togglable bug fixes in a game with more deeply rooted bugs would seriously go out of hand very quickly.

louist103 · 2022-06-08T23:00:25Z

soh/src/overlays/actors/ovl_En_Tk/z_en_tk.c

+#define COLLECTFLAG_GRAVEDIGGING_HEART_PIECE 0x19
+#define ITEMGETINFFLAG_GRAVEDIGGING_HEART_PIECE 0x1000
+
+bool heartPieceSpawned;


This needs to be added to the save states. I'd also give it a more specific name so there are no clashes and make it a s32 type

Hmm, can I add that to save states? It's a local variable to that actor right? Or did I inadvertently make a global variable when I did that.

louist103

Nice job with this.

* Fixes the Gravedigging Tour heartpiece bug. Basically just causes Dampe's Gravedigging Tour Heart Piece to set a Collect flag on the Graveyard Scene when collected instead of a GetItemInf flag when it's spawned. I did this by simply the result of Item_DropCollectible to a variable called reward and running reward->collectibleFlag = 0x19 if the reward was a heartpiece. There may be a better way to do this. This is unlike most of the other dropped items with collectible flags in the game, which have some binary operations performed on the item to be dropped before passing it into Item_DropCollectible. See z_en_geldb.c and z_bg_haka_tubo.c for examples of this. I tried to find some way to do something more like that here but I was unable to wrap my head around the binary operations being performed. I may revisit this in the future. * Reimplements vanilla bug, adds cvar and checkbox for the fix. * Adds some newlines to the Tooltip * Shortens ImGui tooltip. * Removes the hardcoded Purple Rupee/Heart Piece reward. * Sets collectibleFlag whether cvar is on or not to prevent duping. * Sets Gravedigging Tour Fix to enabled by default * Simplifies logic for whether or not to spawn heart piece * Adds TempClear flag set and check for heart piece. This originally introduced a bug where the player could spawn multiple heart pieces by simply not collecting the one that spawns and continuing to dig up spots. This fixes that by checking a temp clear flag before spawning the heart piece and setting it when the heart piece spawns. Since this is a temp clear flag it will not stay set if the player exits the scene, so this still does fix the bug of locking the player out of the heart piece when spawning it and leaving without picking it up. As far as I can tell this temp clear flag isn't used anywhere else in this scene. The only one used in this scene I could find is that killing the first Poe in this scene sets flag 0x02 (or maybe it's 0x01, not sure if the flags start at 1 or 0). * Replaces magic numbers with constants defined in z_en_tk.h * Updates comment explaining changed code. * Replaces another magic number I forgot to replace last commit. * Replaces TempClear flag with local variable * Removes TempClearFlag const and moves others out of .h to .c (felt like they made more sense there)

leggettc18 added 3 commits May 19, 2022 21:43

Merge branch 'HarbourMasters:develop' into gravedigging-tour-fix

f71470a

Reimplements vanilla bug, adds cvar and checkbox for the fix.

ff4966f

Adds some newlines to the Tooltip

de7800a

leggettc18 added 3 commits May 20, 2022 21:47

Shortens ImGui tooltip.

4ea8ee1

Removes the hardcoded Purple Rupee/Heart Piece reward.

fdcb59e

Sets collectibleFlag whether cvar is on or not to prevent duping.

8f30423

leggettc18 added 2 commits May 21, 2022 13:34

Merge changes from upstream develop branch.

46f21df

Merge branch 'develop' into gravedigging-tour-fix

8c06bb7

Sets Gravedigging Tour Fix to enabled by default

c4e0a25

MelonSpeedruns requested review from KiritoDv, PurpleHato and MelonSpeedruns May 25, 2022 18:35

MelonSpeedruns approved these changes May 25, 2022

View reviewed changes

PurpleHato approved these changes May 25, 2022

View reviewed changes

mzxrules reviewed May 27, 2022

View reviewed changes

Merge branch 'develop' of https://github.com/HarbourMasters/Shipwright …

b33a66c

…into gravedigging-tour-fix

leggettc18 added 3 commits May 29, 2022 12:17

Simplifies logic for whether or not to spawn heart piece

ead1342

Merge branch 'develop' of https://github.com/HarbourMasters/Shipwright …

c48e062

…into gravedigging-tour-fix

leggettc18 added 3 commits May 29, 2022 13:17

Replaces magic numbers with constants defined in z_en_tk.h

40d3802

Updates comment explaining changed code.

2c2c48f

Merge branch 'develop' of https://github.com/HarbourMasters/Shipwright …

eee63d4

…into gravedigging-tour-fix

leggettc18 added 3 commits May 29, 2022 16:45

Replaces another magic number I forgot to replace last commit.

6dc5b05

Replaces TempClear flag with local variable

f18a31b

Removes TempClearFlag const and moves others out of .h to .c (felt li…

5da8ff5

…ke they made more sense there)

Kenix3 approved these changes Jun 6, 2022

View reviewed changes

Kenix3 requested review from louist103 and NEstelami June 8, 2022 21:59

louist103 reviewed Jun 8, 2022

View reviewed changes

louist103 requested changes Jun 8, 2022

View reviewed changes

Kenix3 merged commit bf0935a into HarbourMasters:develop Jun 17, 2022

leggettc18 deleted the gravedigging-tour-fix branch August 28, 2022 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gravedigging tour fix #388

Gravedigging tour fix #388

leggettc18 commented May 20, 2022

jbodner09 commented May 20, 2022

leggettc18 commented May 20, 2022

leggettc18 commented May 20, 2022

PurpleHato commented May 20, 2022 •

edited

Loading

leggettc18 commented May 20, 2022 •

edited

Loading

leggettc18 commented May 20, 2022 •

edited

Loading

jbodner09 commented May 20, 2022

leggettc18 commented May 20, 2022 •

edited

Loading

leggettc18 commented May 21, 2022

leggettc18 commented May 21, 2022

leggettc18 commented May 21, 2022

InfoManiac742 commented May 21, 2022

leggettc18 commented May 21, 2022

MelonSpeedruns left a comment

PurpleHato left a comment

mzxrules May 27, 2022

leggettc18 May 27, 2022 •

edited

Loading

mzxrules May 29, 2022 •

edited

Loading

leggettc18 May 29, 2022

leggettc18 commented May 29, 2022

mzxrules commented May 29, 2022 •

edited

Loading

PurpleHato commented May 29, 2022

leggettc18 commented May 29, 2022

leggettc18 commented May 29, 2022

leggettc18 commented May 29, 2022

leggettc18 commented May 29, 2022

jbodner09 commented May 29, 2022

SomeoneIsWorking commented Jun 2, 2022

louist103 Jun 8, 2022 •

edited

Loading

leggettc18 Jun 8, 2022

louist103 left a comment

Gravedigging tour fix #388

Gravedigging tour fix #388

Conversation

leggettc18 commented May 20, 2022

jbodner09 commented May 20, 2022

leggettc18 commented May 20, 2022

leggettc18 commented May 20, 2022

PurpleHato commented May 20, 2022 • edited Loading

leggettc18 commented May 20, 2022 • edited Loading

leggettc18 commented May 20, 2022 • edited Loading

jbodner09 commented May 20, 2022

leggettc18 commented May 20, 2022 • edited Loading

leggettc18 commented May 21, 2022

leggettc18 commented May 21, 2022

leggettc18 commented May 21, 2022

InfoManiac742 commented May 21, 2022

leggettc18 commented May 21, 2022

MelonSpeedruns left a comment

Choose a reason for hiding this comment

PurpleHato left a comment

Choose a reason for hiding this comment

mzxrules May 27, 2022

Choose a reason for hiding this comment

leggettc18 May 27, 2022 • edited Loading

Choose a reason for hiding this comment

mzxrules May 29, 2022 • edited Loading

Choose a reason for hiding this comment

leggettc18 May 29, 2022

Choose a reason for hiding this comment

leggettc18 commented May 29, 2022

mzxrules commented May 29, 2022 • edited Loading

PurpleHato commented May 29, 2022

leggettc18 commented May 29, 2022

leggettc18 commented May 29, 2022

leggettc18 commented May 29, 2022

leggettc18 commented May 29, 2022

jbodner09 commented May 29, 2022

SomeoneIsWorking commented Jun 2, 2022

louist103 Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

leggettc18 Jun 8, 2022

Choose a reason for hiding this comment

louist103 left a comment

Choose a reason for hiding this comment

PurpleHato commented May 20, 2022 •

edited

Loading

leggettc18 commented May 20, 2022 •

edited

Loading

leggettc18 commented May 20, 2022 •

edited

Loading

leggettc18 commented May 20, 2022 •

edited

Loading

leggettc18 May 27, 2022 •

edited

Loading

mzxrules May 29, 2022 •

edited

Loading

mzxrules commented May 29, 2022 •

edited

Loading

louist103 Jun 8, 2022 •

edited

Loading