colyseus / schema

An incremental binary state serializer with delta encoding for games.
https://docs.colyseus.io/state/schema/
MIT License
134 stars 40 forks source link

ArraySchema push/splice/push, item added at wrong index. #94

Open Zielak opened 4 years ago

Zielak commented 4 years ago

I've made this bug reproducible at State handling example of colyseus-example: https://github.com/Zielak/colyseus-examples - branch master.

My use case

I want mark cards selected by the player in their hand. I'm using cardsSelected:ArraySchema<number>. Values are IDs of cards, and i wanted to use the array's index to know in which order the cards were selected.

image


The issue appeared when I tried to select and deselect the same card many times. I've pinned it down to minimal, reproducible case: add/remove/add-again:

image

Note:

I've also inspected web socket messages on client, and it seems that the server-side Colyseus is sending this incorrect key.

endel commented 4 years ago

Thanks for reporting and providing the example @Zielak, thanks also @Steditor for providing a similar scenario here https://github.com/colyseus/colyseus.js/issues/78#issuecomment-724973641 (I believe the use cases are the same)

This issue is going to be a bit difficult to solve. The "dynamicIndex" is going to keep increasing for new records in the array, no matter if you remove items from it or not.

You can get items from the ArraySchema from its "dynamicIndex" - this is not going to be enough, though:

arraySchema.getByIndex(dynamicIndex)

The ArraySchema indexes are very tricky to get right because of the many operations that could happen at once (.splice(), .shift(), .pop(), .push()), that's why I had to use an internal mapping of index -> value for it. There is also the @filterChildren() where local indexes for one client can differ from each other.

It might be possible to work around this somehow and provide the actual local index for the schema callbacks, I just don't want to overly complicate things, as they're already quite complicated! 😰

Zielak commented 4 years ago

So this dynamicIndex is going to keep increasing indefinitely. Can this become a concern in some cases, like: a constantly changing array in long-lasting games?

Anyway, I chose to work around this by having an array of Schemas instead and not rely on the arrays' indexes: [{ selectionIndex: 0, childIndex: 3 }] - 4th card in my hand was selected first :)

endel commented 4 years ago

Glad you've worked it out @Zielak

So this dynamicIndex is going to keep increasing indefinitely. Can this become a concern in some cases, like: a constantly changing array in long-lasting games?

Hm, well observed! Only if it surpasses Number.MAX_SAFE_INTEGER (9007199254740991). Somebody actually may have a use-case to break this 😅

Honga1 commented 3 years ago

Could this be possibly related? https://github.com/colyseus/colyseus/issues/407