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

fix(traps): Fixed traps like the Poisonous Vine being triggered more than once #2464 #2466

Merged

Conversation

JoelFernandes09
Copy link
Contributor

@JoelFernandes09 JoelFernandes09 commented Aug 2, 2023

So I got to fixing it but its a bigger issue overall that I noticed with Traps. After debugging for a bit I found the issue to be in the onStepOut function of the game.

Currently we're applying traps to all the hexes but we want them to be a single trap & not be called multiple times, so the broader solution to this is to maybe make traps occupy different number of hexes but being one entity. This will make it so that all trap related functions etc. will be executed once as intended but the trap will just take on as much space as the creature it was applied on.

For now I've made changes to the triggerTrap function & updated it to the Trap class since it was using a depreciated call as well as made it so the trap activates once but destroys all instances of the trap the creature is currently on. This fix should work temporarily but I feel the above change to traps is something to resolve the issue completely. If that's the case I would be happy to take it on once the issue is created for it. [Fixes #2464]

@vercel
Copy link

vercel bot commented Aug 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
ancientbeast ✅ Ready (Inspect) Visit Preview Aug 15, 2023 5:47pm

@ghost
Copy link

ghost commented Aug 2, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

src/game.ts Outdated
triggeredCreature.hexagons.forEach((hex) => {
hex.activateTrap(this.triggers[trigger], triggeredCreature);
});
const traps = getPointFacade().getTrapsAt(triggeredCreature);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @DreadKnight can chime in here.

From my point of view:

This will destroy all Traps under a Creature as soon as one Trap is activated. I don't think that's desired for all types of Trap – "Poisonous Vine" is a bit of an outlier.

For now, I think this bug should probably be addressed locally in src/abilities/Impaler.js in the "Poisonous Vine" ability.

Copy link
Member

Choose a reason for hiding this comment

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

@andretchen0 I did some testing, with red player's Impaler having upgraded Poisonous Vine on two units and one eventually destroyed the trap, only the trap under it got destroyed, not for the other unit as well. Might do more testing though. Found a bug already, but not directly related to this.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be addressed in src/abilities/Impaler.js as well until we get that higher level fix.

@DreadKnight
Copy link
Member

DreadKnight commented Aug 8, 2023

@JoelFernandes09

Feel free to open that bigger issue yourself about trap sizes and even fix it when you can along with @2464 🐻

@DreadKnight DreadKnight marked this pull request as draft August 8, 2023 13:55
@DreadKnight
Copy link
Member

@JoelFernandes09 Heya! As @andretchen0 said, "I think this bug should probably be addressed locally in src/abilities/Impaler.js in the Poisonous Vine ability."; feel free to patch and mark this ready for review.

@JoelFernandes09
Copy link
Contributor Author

Will do! @DreadKnight

@JoelFernandes09
Copy link
Contributor Author

@DreadKnight Fixed this by adding a bool check so the trap is placed once with the effect & the others are just placeholders with no effect. I'll open the bigger issue with traps as discussed above soon!

@JoelFernandes09 JoelFernandes09 marked this pull request as ready for review August 15, 2023 17:35
@DreadKnight
Copy link
Member

@JoelFernandes09 Cool, will check it out soon!

@DreadKnight DreadKnight merged commit 6dc4c89 into FreezingMoon:master Aug 16, 2023
3 checks passed
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.

only one notification for Poisonous Vine [bounty: 2 XTR]
3 participants