FreezingMoon / AncientBeast

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

Creature ids shouldn't be duplicated, should be consistent #2163

Closed andretchen0 closed 1 year ago

andretchen0 commented 1 year ago

The id field on /src/creature.js is updated inconsistently.

Sometimes the same id is used for multiple creatures. And sometimes for the same creature, the id changes.


How to see creature id's

One way to see creature id's in the console would be to paste this ...

console.log(this.game.queue.queue.map(c=>c.id), this.game.queue.nextQueue.map(c=>c.id));

... into this function


Same id for multiple creatures

To reproduce:

  1. Open the game and paste the AB.restoreGame([...]) from here into the console.
  2. One of the Priests will have id 1. All other creatures will have id 2.

Changing id for same creature

To reproduce:

  1. Start a game
  2. Using a Priest, choose to summon a creature.
  3. Choose a creature from the dash.
  4. Place the creature on the board.
  5. That creature's id will vary between n and n+1 during the operation.

Expected behavior Id's for creatures should remain consistent for a creature and not shared between creatures.

andretchen0 commented 1 year ago

Looking at the code and gameplay, it doesn't appear that it's possible for a player to create 2 separate instances of the same creature. If that's the case, then [creature type | team] could form a kind of id.

That fixes that rule (no duplicate creatures) way down deep in the system, so maybe it's not the best way forward. At the same time, it's a relatively easy switch.

On the other hand, it's quite a challenge to track down exactly how creature creation info is threaded through the system atm.

Thoughts?

andretchen0 commented 1 year ago

I think I'm missing something with game.creatures. It's an array, but it's treated like a creature.id -> creature map sometimes. Creatures store themselves in game.creatures at game.creatures[creature.id]. See:

https://github.com/FreezingMoon/AncientBeast/blob/19f13d4a68e291e75b774f69ae92aa5ffd2000af/src/creature.js#L261

This seems to lead to some complications:

Can you tell me what the rationale is for doing it this way? Can game.creatures simply be used as an array? I.e., not necessarily store creature at game.creatures[creature.id]?

DreadKnight commented 1 year ago

Looking at the code and gameplay, it doesn't appear that it's possible for a player to create 2 separate instances of the same creature. If that's the case, then [creature type | team] could form a kind of id.

That fixes that rule (no duplicate creatures) way down deep in the system, so maybe it's not the best way forward. At the same time, it's a relatively easy switch.

On the other hand, it's quite a challenge to track down exactly how creature creation info is threaded through the system atm.

Thoughts?

@andretchen0 There were plans at some point to have Sarcophagus ultimate to summon its own unit https://ancientbeast.com/documentation/?id=units#Sarcophag That's actually listed right under https://ancientbeast.com/documentation/?id=units#Shadow_Leech which could be created multiple times; also Vehemoth (previously called Ice Demon) at some point was designed to be able to create non-movable (0 movement and auto-turn) snow men entities that would hit inline enemy with snowball. Anyway, avoided that sort of stuff for now, but the snow-men could be a thing for another sloth unit from upcoming set.

DreadKnight commented 1 year ago

I think I'm missing something with game.creatures. It's an array, but it's treated like a creature.id -> creature map sometimes. Creatures store themselves in game.creatures at game.creatures[creature.id]. See:

https://github.com/FreezingMoon/AncientBeast/blob/19f13d4a68e291e75b774f69ae92aa5ffd2000af/src/creature.js#L261

This seems to lead to some complications:

  • creature.id begins at 1, so the first spot in game.creatures is empty, meaning now the creatures can't be iterated over without filtering first.
  • with temp creatures that are cancelled – not placed on the board – the system has to decrement the creature.id number, or else the next time a creature is created, it'll leave a hole in game.creatures.

Can you tell me what the rationale is for doing it this way? Can game.creatures simply be used as an array? I.e., not necessarily store creature at game.creatures[creature.id]?

Well, first prototype was created by a French artist that was decent coder (unlike me) but had no coding education nor plans to pursue a coding career, but he ended up working on Blender 3d regardless. Some of the creatures were 3d modeled by him using Blender. A lot of the coding used to have very bad English or be weird or inefficient, but he did things quite fast and I'm glad he actually got stuff done. So if some of the coding is weird or bad, we can poke at it, though we should consider the project as a "game engine" of sorts (and I don't mean Phaser stuff), but for TBS genre, so that it could be forked and made into a completely different universe / timeline and such, idea is to not super restrict mechanics.

V0.1 had very small raster, so we were afraid that we might have to allow duplicated units simultaneously or at least be able to rematerialize units; we allowed each player to have its own raster at least instead of sharing, which might be how vanilla combat mode might work at some point once the first unit set is complete, meaning one unit per combat, first come first served. Regardless of the small unit raster of the first playable prototype, we were actually very amazed how replayable it was even at that early stage. I'm probably ranting as these aren't the answers you're expecting, but at least giving you some Ancient (Beast) history and to whomever might wonder around seeking it one day. Snapshot of project did end up in the Arctic Code Vault 😎 so that sort of stuff could happen again one day.

andretchen0 commented 1 year ago

TLDR, storing creatures at game.creatures[creature.id] seems to just be creating extra bookkeeping, without any upside. I'd like to modify that and make the index in game.creatures meaningless (aside from being a normal array index). Do you foresee problems with that?

As it stands, the indices are the creature.id, but this means there are holes in the array from the beginning, since creature.id start at 1. And when creatures are removed from the array, their spot in the array remains empty, unless the id is reused. Some parts of the codebase reuse creature ids, but then other parts are forced to disambiguate that.

If you're not sure if the change will create problems, then that's ok. I just figured it'd be best to ask first.

DreadKnight commented 1 year ago

TLDR, storing creatures at game.creatures[creature.id] seems to just be creating extra bookkeeping, without any upside. I'd like to modify that and make the index in game.creatures meaningless (aside from being a normal array index). Do you foresee problems with that?

As it stands, the indices are the creature.id, but this means there are holes in the array from the beginning, since creature.id start at 1. And when creatures are removed from the array, their spot in the array remains empty, unless the id is reused. Some parts of the codebase reuse creature ids, but then other parts are forced to disambiguate that.

If you're not sure if the change will create problems, then that's ok. I just figured it'd be best to ask first.

So should have Dark Priest be id 0 and move some other unit into id 1 or they're not the same ids from the data json file? If stuff doesn't affect some possible match player that would be able to scroll back and forward or mechanics for mods, it's fine with me. Current way of doing things might just be poorly designed probably, stuff gets tested anyway so we'll see 🤞🏻

andretchen0 commented 1 year ago

So should have Dark Priest be id 0 and move some other unit into id 1 or they're not the same ids from the data json file?

Coming in from the outside, my first guess would be that a creatures array would simply hold the creatures currently in the game. While it does hold the creatures, they're ordered in a particular way, with undefined interspersed in between sometimes. That means, e.g., you can't map over creatures without filtering first.

If the old way of doing things is necessary for some purpose, it's easy enough to put back together with a reduce.

andretchen0 commented 1 year ago

@DreadKnight

I propose removing game.creatureIdCounter. Does that raise any concerns with you?

If that sounds ok or if I don't hear back, I'll submit a PR.

DreadKnight commented 1 year ago

@DreadKnight

I propose removing game.creatureIdCounter. Does that raise any concerns with you?

If that sounds ok or if I don't hear back, I'll submit a PR.

We can give it a try and see how it goes during testing and such.