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

[Enhancement] EasyMaskEquip #755

Open
wants to merge 75 commits into
base: develop
Choose a base branch
from

Conversation

mckinlee
Copy link
Contributor

@mckinlee mckinlee commented Aug 11, 2024

EasyMaskEquip is a feature that allows players to quickly equip masks without needing to assign them to C-buttons or D-pad slots. It simplifies the process by letting players equip masks directly from the pause menu. The enhancement also provides visual feedback by highlighting the currently equipped mask and graying out masks that can't be equipped in certain situations (e.g., underwater or mid-transformation).

This PR introduces the EasyMaskEquip enhancement, allowing players to equip masks directly from the pause menu by pressing the A button.

A new OnKaleidoClose hook was added to handle mask equipping when the pause menu is closed. This allows players to change their mask selection if they accidentally select the wrong one, ensuring the mask is only equipped once the pause menu is closed.

This enhancement also utilizes Proxy's BeforeKaleidoDrawPage hook from PR #537 to allocate and update vertex data for the mask equip border.

Build Artifacts

@mckinlee mckinlee changed the title [Enhancement] [WIP] EasyMask Equip [Enhancement] [WIP] EasyMaskEquip Aug 11, 2024
@mckinlee
Copy link
Contributor Author

I think this is already in a pretty good working state so I'm just gonna go ahead and change this to ready for review. I don't foresee this having to many issues, but please do report any bugs you may find.

@mckinlee mckinlee marked this pull request as ready for review August 11, 2024 18:42
@mckinlee mckinlee changed the title [Enhancement] [WIP] EasyMaskEquip [Enhancement] EasyMaskEquip Aug 11, 2024
… any mask in the air when wearing a transformation mask
- Streamlined mask selection logic with helper functions
- Added grayscale indicators for unavailable masks in the UI
- Optimized vertex handling for mask border rendering
- Improved game hook integrations for reliable mask equipping
- Enhanced code readability and maintainability
@mckinlee
Copy link
Contributor Author

I realize ShouldEquipMask may not account for all scenarios, but its a good start. I would need help identifying what else we may need to add.

@mckinlee
Copy link
Contributor Author

63 commits later and I think I've gotten this to a state I'm happy with. I don't plan to make any more changes unless the devs request otherwise. Hope yall like it!

@mckinlee
Copy link
Contributor Author

Previously, Easy Mask Equip supported both regular and transformation masks, which forced disabling some vanilla behaviors and risked breaking existing logic. To avoid discarding all my work, this update refocuses the feature solely on transformation masks as an extension of Fast Transformation. This change simplifies the implementation via a clear state machine for handling pending actions and frees up the C and D pad buttons for other items, preserving the intended quality-of-life improvements.

Copy link
Contributor

@garrettjoecox garrettjoecox left a comment

Choose a reason for hiding this comment

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

Though I know the interface is similar to the persistent bunny hood, the user having to unpause to engage the transformation just feels awkward, I'm not really sure how I feel about this one.


// Hook for controlling the display of the selected item textbox.
COND_VB_SHOULD(VB_KALEIDO_DISPLAY_ITEM_TEXT, CVAR, {
*should = !isTransformationMask(static_cast<ItemId>(gPlayState->pauseCtx.cursorItem[PAUSE_MASK]));
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be causing an issue with persistent bunny hood, causing it to show the text when toggling it.

Instead I think you want

Suggested change
*should = !isTransformationMask(static_cast<ItemId>(gPlayState->pauseCtx.cursorItem[PAUSE_MASK]));
if (isTransformationMask(static_cast<ItemId>(gPlayState->pauseCtx.cursorItem[PAUSE_MASK]))) {
*should = false;
}


/// Returns true if the given mask is a transformation mask.
bool isTransformationMask(ItemId mask) {
return std::binary_search(kTransformationMasks.begin(), kTransformationMasks.end(), mask);
Copy link
Contributor

Choose a reason for hiding this comment

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

there are so little masks here it's probably more simple to just do a direct comparison inline than store them in an array and iterate over it.

Comment on lines 53 to 56
for (s16 slot = 0; slot < MASK_NUM_SLOTS; ++slot) {
if (items[slot + ITEM_NUM_SLOTS] == target)
return slot;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the SLOT() macro here should get you what you want (I think?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The gItemSlots array maps itemIDs to their slots, but looks like the mapping is only applicable for regular item maybe? I couldn't get it to work with the masks.

@mckinlee
Copy link
Contributor Author

mckinlee commented Apr 3, 2025

Though I know the interface is similar to the persistent bunny hood, the user having to unpause to engage the transformation just feels awkward, I'm not really sure how I feel about this one.

I pushed an update that not only addresses some of you feedback but also goes a bit overboard with a pretty extensive refactor. The changes include streamlining state management with a scoped enum and a dedicated state tracker, introducing a new VertexBuffer for more robust vertex handling, and encapsulating input and state logic into a new MaskEquipManager. The interface has got a upgrade as well and should hopefully address that awkwardness you mentioned.

Fair warning: it's a pretty hefty update, so you might want to grab a coffee before diving in lol. Let me know if you have any questions or need further tweaks!

@mckinlee mckinlee requested a review from garrettjoecox April 3, 2025 21:01
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.

2 participants