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

V1 - Chestapalooza #2922

Merged
merged 9 commits into from
Aug 30, 2023
Merged

V1 - Chestapalooza #2922

merged 9 commits into from
Aug 30, 2023

Conversation

Caladius
Copy link
Contributor

@Caladius Caladius commented May 25, 2023

This is as good as my skill set could get for a V1. Sending a PR in hopes of some code review and helpful pointers for cleanliness.

Everything is operational in its current state, and each trap can be toggled off which will revert it back to a normal Ice Trap.

Freeze Trap
Shock Trap
Burn Trap
Explosion Trap
Knockback Trap
Teleport Trap
Void Trap
Death Trap
Ammo Consumption Trap
Speed Trap

I'll add more in V2 or V3. hoping to utilize some future commits i see in the works to enable custom trap messages per trap type.

Build Artifacts

Copy link
Contributor

@Malkierian Malkierian left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good. Unfortunately I can't test it because of merge conflicts, so it isn't building. You'll need to pull the latest changes manually and address those merge conflicts.

soh/soh/Enhancements/mods.cpp Outdated Show resolved Hide resolved
switch (roll) {
case 1:
if (!CVarGetInteger("gChestIce", 0)) {
GameInteractor::RawAction::FreezePlayer();
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to that, you could then handle the determination of what effect to apply with a unified function GameInteractor::RawAction::PlayerEffect or maybe DamageEffect, not sure. This would then just accept an effect enum value and apply the correct one internally. This is more of a suggestion, and probably down to preference, but other devs who have worked with this project more could probably chime in.

soh/src/overlays/actors/ovl_En_Box/z_en_box.c Outdated Show resolved Hide resolved
soh/soh/Enhancements/mods.cpp Show resolved Hide resolved
soh/src/overlays/actors/ovl_player_actor/z_player.c Outdated Show resolved Hide resolved
garrettcoxappfolio

This comment was marked as duplicate.

@garrettjoecox
Copy link
Contributor

I like the cutesy name but it's not intuitive 🗡️

I'd replace both the method/display name as well as the cvars to something more clear, and you can nest the cvars.

Something like
Alternative Chest Traps
RegisterAltChestTraps()
gAltChestTraps.Enabled
gAltChestTraps.IceTrapEnabled
gAltChestTraps.BurnTrapEnabled

etc, doesn't have to be this but should be clear on what it is.

@garrettjoecox
Copy link
Contributor

Just so it's not lost in DM's, I'd also recommend building out an array of available traps and choosing one from the available array so there is an even distribution (I sent you the code snippet PoC for this)

@Malkierian
Copy link
Contributor

Just so it's not lost in DM's, I'd also recommend building out an array of available traps and choosing one from the available array so there is an even distribution (I sent you the code snippet PoC for this)

Did you see my suggestion about the enum and such? I think we were talking about the same section, just wondering if we had the same idea, or our ideas would supplement each other or what.

@garrettjoecox
Copy link
Contributor

Just so it's not lost in DM's, I'd also recommend building out an array of available traps and choosing one from the available array so there is an even distribution (I sent you the code snippet PoC for this)

Did you see my suggestion about the enum and such? I think we were talking about the same section, just wondering if we had the same idea, or our ideas would supplement each other or what.

yup I think we were on the same page there 👍

soh/soh/Enhancements/mods.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/mods.cpp Outdated Show resolved Hide resolved
@garrettjoecox garrettjoecox added this to the MacReady milestone Jun 13, 2023
Comment on lines +106 to +108
Player* player = GET_PLAYER(gPlayState);
player->actor.colChkInfo.damage = 0;
func_80837C0C(gPlayState, player, 3, 0, 0, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change was required because the ice traps can now be processed by game interactor, so this caused a infinite loop of ice traps. Not sure if there is a better way to handle this..

Co-authored-by: Garrett Cox <garrettjcox@gmail.com>
@garrettjoecox
Copy link
Contributor

@briaguya-ai / @leggettc18 / anyone this is ready for another look. Me and cal paired and got it working from all check types with a new hook but I'm not sure how I feel about the solution yet. Looking for feedback

Copy link
Contributor

@PurpleHato PurpleHato left a comment

Choose a reason for hiding this comment

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

From what I can see, it looks good and doesn't seem to complicate the existing codebase. It would be helpful to conduct some playtesting to ensure its functionality and effectiveness. So I would say LGTM so far

Overall, this pr appears to be in good shape and adds something that I was looking forward. Thank you for your work on this!

More tweaks, remove processed trap hook
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.

Did another pass on this to remove the new hook and just use OnItemReceive, as well as clean up how ice traps emit the OnItemReceive hook

This has been playtested for a month or so now on Anchor. Would be nice to get at least one other maintainers' eyes on it since some of it is my code.

@Malkierian Malkierian mentioned this pull request Aug 20, 2023
@garrettjoecox garrettjoecox merged commit 0e7c658 into HarbourMasters:develop Aug 30, 2023
8 checks passed
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.

None yet

7 participants