Crash when dragging a condition into an OR, NOT, AND condition

[victorlevasseur]

(http://forum.compilgames.net/viewtopic.php?f=20&t=5111)
The bug seems to happen in gd::EventEditorSelection::EndDragInstruction : at the end, when GD is trying to show (in std::cout) the list events type moved.
If I remove that part, GD crash somewhere else...

[victorlevasseur]

Some news on this bug ? Seems that a gd::Instruction contained inside list is invalid (causing i->GetType() failure).

[4ian]

The bug happen when dragging an instruction from a list into a OR/AND/NOT condition from the same list: as removing the dragged condition implies that the list is changed, every instruction are changed and so the pointer to the list where the condition is dropped become invalid..
The problem does not happen with events as they are managed by (shared) pointers and not by value (so any reference to an event remains valid even if it is moved).

Maybe I should prevent any drag'n'drop to a subcondition list for now.

[victorlevasseur]

Or return NULL instead of the list.

[victorlevasseur]

Should we convert instructions lists to instruction's pointers lists ? (Is this easy to do ?)

[4ian]

Well, instead of having list of instructions represented as std::vector < gd::InstructionItem > we should have an gd::InstructionsList class (just like we manage events using gd::EventsList).
Internally, this class will manage instructions in a std::vector but not by value as now but using smart pointers (std::shared_ptr<Instruction>) => this enables to swap/move/reorder instructions in the list (or even move instructions from list to another list) without losing the reference to it.

Currently, as they are stored by value, any change in a list has the potential to invalidate all the list (for example if the list is reallocated) so we can't keep any pointer (or iterator) on an element of the list.
This is fine when you're just dealing with list of things: do a copy of whatever you want to move, and swap/move it using the index. Just don't keep pointers on the Instruction because any change in the vector can trigger reallocation and invalidate all pointers or iterators.

But in GDevelop instructions can have subinstructions, so in fact we are not dealing with simple arrays/lists but with a tree of instructions. If you try to move an instruction into a list of a subinstructions, you potentially risk, when you add/remove instructions, to invalidate all the pointers.

This is what is happening and triggering the bug: dragging instructions in the events editor triggers change in a the instruction list of this instruction. If you try to drop the instruction in a list that changed boooom everything explodes.

Makes sense? A bit hard to get surely at first. Put it another way, here is the current problem:

You have a list of instructions and each instructions have a list of (sub)instructions:

Instruction1
Instruction2
   -Subinstruction1
   -Subinstruction2
   -Subinstruction3
Instruction3 

When you want to drag for example Instruction1 and Instruction3 after Subinstruction2, who do you proceed? Knowing that any add/remove operation on a list will (potentially) reallocate it and make all pointers/reference/iterator to it invalid.

[victorlevasseur]

I think we can create a template class SPtrList and use it for the EventsList and the InstructionsList. The problem is the copy ctor : they are not the same. EventsList uses CloneRememberingOriginalEvent to copy the event and InstructionsList will probably use the copy ctor of each Instruction.

[victorlevasseur]

See the branch bugfix/instructions-list in my fork.
I've created a template class to manage a vector of shared_ptr. The InstructionsList is just a typedef of it for gd::Instruction. I may also use it for the EventsList but not as a simple typedef as it has some other methods (serialization and recursive search).

[4ian]

Taking a look at it. I think that EventsList can be kept as it is now! :)

[4ian]

Good job! The code looks great, good use of make_shared, comments and methods names are consistent with the other GD classes. Really nice what you came up with :)

As I said EventsList can be kept separately, no need to bother with it I think - if we want to refactor it later it won't be much difficult as you class is generic.

Great job, it's better than anything I was dreaming of. 😄
Send me a PR and I'll merge it if you think everything is ok. When done I'll take a look again and this bug to see if I can rewrite the drag'n'drop method. :)

[victorlevasseur]

I don't see why you want to rewrite the d'n'd method as it's not crashing anymore with a list of shared_ptr.
(for the interface, I've just copied gd::EventsList and renove "Event" from the method's names).

[4ian]

Ah oO Cool cool! I thought it could still be crashing because even if the implementation of the lists changed, the semantic and usage of lists is still the same (so it could have lead to the same issues with using deleted instructions).

So that just perfect, thanks a lot! I'm waiting for the PR 😄