search

FreezingMoon / AncientBeast

The Turn Based Strategy Game/eSport. Master your beasts! 🐺
https://AncientBeast.com
GNU Affero General Public License v3.0
1.66k stars 569 forks source link

Fixes issue https://github.com/FreezingMoon/AncientBeast/issues/2494 #2508

Closed tomaszajac-vs closed 11 months ago

tomaszajac-vs commented 11 months ago

This fixes issue #2494

vercel[bot] commented 11 months ago

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

Name Status Preview Updated (UTC)
ancientbeast ✅ Ready (Inspect) Visit Preview Oct 18, 2023 8:16am
ghost commented 11 months ago
👇 Click on the image for a new way to code review #### [![Review these changes using an interactive CodeSee Map](https://s3.us-east-2.amazonaws.com/maps.codesee.io/images/github/FreezingMoon/AncientBeast/2508/0e90fad8/dd6f28f986e59e64ee6403c247201b01e39b11c9.svg)](https://app.codesee.io/r/reviews?pr=2508&src=https%3A%2F%2Fgithub.com%2FFreezingMoon%2FAncientBeast) #### Legend CodeSee Map legend
allmtz commented 11 months ago

@tomaszajac-vs Great work!

A couple of things to consider:

I think it would be less error prone to have flipped only exist on player. Duplicating it on player and creature introduces the possibility of these two getting out of sync.

The logic inside getHexMap and getHexLine will most likely need to be refactored. I haven't tested the current changes but last time I tried passing the correct value of flipped to getHexMap and getHexLine, a lot of the targeting functionality broke.

tomaszajac-vs commented 11 months ago

Do you propose to only use the flipped property from the player for all of the creatures?

DreadKnight commented 11 months ago

I need to keep in mind what @allmtz said, duplicated stuff that can get out of sync is a bad thing indeed. When I'll test this soon will have to pay attention to the unit abilities.

allmtz commented 11 months ago

Do you propose to only use the flipped property from the player for all of the creatures?

I think this makes sense. A player is either flipped (i.e. on the right side of the screen) or not (on the left side) and any creatures the player summons will inherit that flipped status.

Each creature already has a player property so acessing flipped would look like creature.player.flipped.

The benefit I can think of in keeping a copy of the flipped value on a creature is for something like a status effect that only effects that creature. Even so, the same could be accomplished by creating something like a isDisoriented flag on creature. Other than this, I don't see a reason to have the flipped status exists on a creature independent from the player that summoned it.

DreadKnight commented 11 months ago

Will mark this as draft until @allmtz's suggestion gets implemented, as it makes a lot of sense to me.

tomaszajac-vs commented 11 months ago

Hi, I have changed the reference to the player.flipped on all of the creatures

DreadKnight commented 11 months ago

@tomaszajac-vs Seems to work fine as far as I tested so far 👍🏻