Skip to content

Conversation

@OtherBlue
Copy link
Contributor

@OtherBlue OtherBlue commented Jan 2, 2026

Copy link
Contributor

@mckinlee mckinlee left a comment

Choose a reason for hiding this comment

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

Great job on this. I did not experience any issues in my testing. Codewise you are looking good, but the new VBs in GameInteractor_VanillaBehavior.h need to be moved above VB_KALEIDO_UNPAUSE_CLOSE to maintain alphabetical order.

@OtherBlue
Copy link
Contributor Author

Great job on this. I did not experience any issues in my testing. Codewise you are looking good, but the new VBs in GameInteractor_VanillaBehavior.h need to be moved above VB_KALEIDO_UNPAUSE_CLOSE to maintain alphabetical order.

done

Copy link
Contributor

@Eblo Eblo left a comment

Choose a reason for hiding this comment

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

Works nicely. My concerns are primarily with code flow and some tidying up.


void RegisterDpadPageSwitchPrevention() {
COND_VB_SHOULD(VB_KALEIDO_SWITCH_PAGE_WITH_DPAD, CVarGetInteger("gEnhancements.Dpad.DpadEquips", 0), {
PlayState* play = va_arg(args, PlayState*);
Copy link
Contributor

Choose a reason for hiding this comment

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

The globally available gPlayState should give you what you need instead of having to pass a PlayState* arg, or is there a reason you had to do it this way?


void RegisterItemUnequip() {
COND_VB_SHOULD(VB_KALEIDO_EQUIP_ITEM_TO_BUTTON, CVAR, {
PlayState* play = va_arg(args, PlayState*);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Comment on lines 1186 to 1188

} else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_DOWN) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_DOWN) {
} else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_DOWN) {

DPAD_SLOT_EQUIP(0, EQUIP_SLOT_D_DOWN) = pauseCtx->equipTargetSlot;
Interface_Dpad_LoadItemIconImpl(play, EQUIP_SLOT_D_DOWN);
} else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_UP) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

}
// #endregion

// Item unequip enhancement
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Item unequip enhancement

DPAD_SLOT_EQUIP(0, EQUIP_SLOT_D_DOWN) = pauseCtx->equipTargetSlot;
Interface_Dpad_LoadItemIconImpl(play, EQUIP_SLOT_D_DOWN);
} else if (pauseCtx->equipTargetCBtn == PAUSE_EQUIP_D_UP) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

return;
}
pauseCtx->equipTargetCBtn = PAUSE_EQUIP_D_UP;
else if (CVarGetInteger("gEnhancements.Dpad.DpadEquips", 0) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble understanding why if (CVarGetInteger("gEnhancements.Dpad.DpadEquips", 0) was moved from being one overarching condition with further nested conditions, to being an AND condition for every case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is from when I was troubleshooting to try and understand why the enhancement didn't wanna work with the dpad and I thought I removed it but ig not????

Comment on lines 737 to 740
// #endregion

// #region 2S2H [Enhancement]
// Item unequip enhancement
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment tag is mostly for inline changes to source code. VB names are self-explanatory, plus this entire DPad input region already labeled as such.

Suggested change
// #endregion
// #region 2S2H [Enhancement]
// Item unequip enhancement

#define CVAR CVarGetInteger(CVAR_NAME, 0)

void RegisterDpadPageSwitchPrevention() {
COND_VB_SHOULD(VB_KALEIDO_SWITCH_PAGE_WITH_DPAD, CVarGetInteger("gEnhancements.Dpad.DpadEquips", 0), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you go ahead and follow the same pattern you did with ItemUnequip? Just so we're not repeating one name and not the other. CVAR_DPAD_EQUIPS is probably as good a name as any.

improved code flow in general and undid unnecessary edit to dpad equips
@OtherBlue OtherBlue marked this pull request as draft January 7, 2026 15:25
@OtherBlue
Copy link
Contributor Author

something broke, will check in a bit

@OtherBlue
Copy link
Contributor Author

found what it was, just didn't update the GameInteractor_Should calls

@OtherBlue OtherBlue marked this pull request as ready for review January 7, 2026 15:50
@OtherBlue
Copy link
Contributor Author

done

Copy link
Contributor

@mckinlee mckinlee left a comment

Choose a reason for hiding this comment

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

Based on changes that addressed Eblo's feedback I have one small change request.

Copy link
Contributor

@mckinlee mckinlee left a comment

Choose a reason for hiding this comment

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

Did some sanity testing. Still works like a charm. I did notice a potential UX concern, but I don't think that's blocking. If a magic arrow is equipped, you cannot unequip it on the ITEM_BOW slot. It requires using the applicable magic item slot only. If you choose to address that, I'll re-review, but I'm approving as is.

@OtherBlue
Copy link
Contributor Author

do you mean to be able to unequip any bow + magic arrow from the bow item? I didn't even think about that possibility tbh... would it be preferable to have it work like that?

@mckinlee
Copy link
Contributor

mckinlee commented Jan 7, 2026

do you mean to be able to unequip any bow + magic arrow from the bow item? I didn't even think about that possibility tbh... would it be preferable to have it work like that?

You know how when you have a item equipped to C/D it draws that white border around the item in the pause menu as a indicator? When you equip a magic arrow, that border is drawn over the ITEM_BOW slot, not specifically the magic arrow slot thats equipped. So my instinct as a player was to try to unequip it using that particular slot. I had gotten use to seeing the white border as a indicator for what can be unequipped for all other items. I hope that makes sense. I'd love to hear other opinions on this, but my first thought was to keep what you have as-is, but additionally allow unequipping the magic arrow with the regular bow slot.

@OtherBlue
Copy link
Contributor Author

that does make sense tbh
yeah I can add it

@OtherBlue
Copy link
Contributor Author

ok... I think that's everything

Copy link
Contributor

@mckinlee mckinlee left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM.

Copy link
Contributor

@Eblo Eblo left a comment

Choose a reason for hiding this comment

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

Looking cleaner now! One final nitpick, but if you can address my two standing cleanup feedback suggestions, I think this will be good to go. Mostly just to reduce the source footprint a little more.

@Eblo Eblo merged commit 0ebb3c8 into HarbourMasters:develop Jan 9, 2026
5 checks passed
@OtherBlue OtherBlue deleted the unequip-c-items branch January 10, 2026 13:38
YoshiCrystal9 pushed a commit to YoshiCrystal9/2ship2harkinian-Switch that referenced this pull request Jan 12, 2026
* Bg_Danpei_Movebg OK

* Cleanup

* PR comments

* Couple small things

* Address the nit

* Flags

---------

Co-authored-by: Derek Hensley <hensley.derek58@gmail.com>
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.

3 participants