Palapeli - MergeGroup and Teleport

[Ian Wadham]

Hi Christian,

Thanks very much for all the work you are doing on Palapeli.

On 10/09/2017, at 8:08 PM, Christian Ehrlicher wrote:
> Palapeli crashes when a piece which gets merged is teleported to another scene. To trigger this in a reproducible way, increase the animation time in MergeGroup::start() to e.g. 2000ms to have time to start the teleport.
> Then create a new view and make sure to mark it as destination of the teleport. After this start a merge and while the merge is ongoing, teleport this piece to the other view.
> You'll get a crash either in GamePlay::updateSavedGame(), GamePlay::transferPieces(), SelectPieceInteractor::stopInteraction(), ... everywhere when accessing the piece in the destination view.
> 
> I've three possible solutions:
> - avoid a teleport during a ongoing merge
> - make sure to get informed when a Piece is destroyed (Piece is derived from QObject so it should be easy)
> - use QPointer<> instead raw pointers and check for != nullptr before accessing the piece in everywhere
> 
> The first one would avoid the crash but I think the other two would also help to prevent the crashes in the bugtracker.
> 
> What do you think?

Quick response only.  I'd like to spend some time doing a detailed walkthrough
of the code, but I don't have much time today.

Your example is like the MergeGroup/restartPuzzle conflict in that it is a useful
test-bed for the code but unlikely to occur in actual play, so I would not be in a
hurry to fix that case.

My instinct is that what we are seeing in all the crashes is due to asynchrony
between the actual merge (at model level), the merge animation and the life
and death of mouse interactor sequences, compounded by Piece being a
complex data structure containing both model and view aspects: "model" being
the location and existence of the piece in whatever scene/view, "view" being the
various PieceVisuals (the picture on the piece, the shadow and optional 3-D bevels).

My idea is that, once a piece has been "committed" to a merge (in a database
management sense) it should be "locked" in some way, before returning to the
Qt event loop, such as by making it non-selectable immediately and until it is
eventually deleted.  This should leave visuals free (in "read-only" mode) for the
animation, but with clicks disabled.

Cheers, Ian W.