Skip to content

Fix crash when dragging card as discard phase ends#37419

Merged
bdach merged 3 commits intoppy:masterfrom
minetoblend:rp-card-drag-fix
Apr 20, 2026
Merged

Fix crash when dragging card as discard phase ends#37419
bdach merged 3 commits intoppy:masterfrom
minetoblend:rp-card-drag-fix

Conversation

@minetoblend
Copy link
Copy Markdown
Contributor

@minetoblend minetoblend commented Apr 20, 2026

Closes: #37402

Issue was HandOfCards.CardContainer.Compare working under the assumption that the it would never be called with the same child for both entries, which can happen when doing a BinarySearch (called in RemoveInternal).
This lead to IndexOf returning a negative value despite the card being present in the container, and the drawable getting disposed but not actually removed.

I also included a precautionary cardContainer.Sort() call before the removal as well, seemed to work without that from testing but better not rely on that.

TLDR: card container child sorting was unstable due to poor assumptions

@bdach bdach added type/reliability Deals with game crashing or breaking in a serious way. area:ranked-play labels Apr 20, 2026
@bdach bdach self-requested a review April 20, 2026 08:21
@bdach
Copy link
Copy Markdown
Collaborator

bdach commented Apr 20, 2026

I gotta say, all this code is playing some very dangerous games as far as I can tell. Not sure that I would have spotted that in review originally but looking at all this from the failure angle there's a lot of dicey.

  • The list potentially being in unsorted state until the next update loop runs. Especially after additions.
  • I don't see any gaps in the invalidation flow that is supposed to re-sort the cards but it does make me rather uneasy that the invalidation is delayed.
  • The fact that HandOfCards.Cards exposes cardContainer.Children is also a bit of a 🙈 moment. It appears to be okay but only because of two facts:
    • it's exposed as IReadOnlyList<T>, so there's no way to access the spicy operations like IndexOf() and such without downcasting which would be the caller's fault
    • I'm not sure why this is the case but re-sorting a drawable's children does not increment the enumeratorVersion of the drawable. It sure looks like an omission, and if it was incremented, this enumerator would be very busted. That said, it's not incremented, and therefore the enumerator is magically safe. I'm not going to go fix this and probably spend the next 2 days chasing the inevitable bugs that this uncovers.

All of which is a long-winded way to say "reluctantly approved for hotfix purposes".

@bdach bdach merged commit 11f0111 into ppy:master Apr 20, 2026
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ranked-play size/M type/reliability Deals with game crashing or breaking in a serious way.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crash in ranked play holding a card

2 participants