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

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

Open
Zielak opened this issue Nov 12, 2020 · 4 comments
Open

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

Zielak opened this issue Nov 12, 2020 · 4 comments
Milestone

Comments

@Zielak
Copy link
Contributor

Zielak commented Nov 12, 2020

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:

  • the array is empty at the beginning,
  • the second onAdd comes with the item being added into index 1

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

@endel
Copy link
Member

endel commented Nov 13, 2020

Thanks for reporting and providing the example @Zielak, thanks also @Steditor for providing a similar scenario here colyseus/colyseus.js#78 (comment) (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
Copy link
Contributor Author

Zielak commented Nov 15, 2020

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
Copy link
Member

endel commented Nov 16, 2020

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
Copy link

Honga1 commented Apr 13, 2021

Could this be possibly related? colyseus/colyseus#407

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

No branches or pull requests

3 participants