Fix #1986

[MSGhero]

Closes #1986

Iterating while removing, change taken from Watch. I'm not sure if copying is the best way vs iterating twice, but this makes it consistent with other parts of flixel.

[seraku24]

This might fall under the category of refactoring, but there is potential for a cleaner and faster solution here.

Having removeAll() defer to remove() is slow, specifically O(_n_²), due to the need to iterate the collection per each element. Since all items are going to be removed, the array can be cleared directly instead of removing elements one at a time. To avoid duplication of code, the existing "null"ing logic currently within remove() should be in a destroy() method that belongs to ObjectMouseData<T>. This would enable both remove() and removeAll() to share this cleanup logic without depending on each other.

(Some hastily written and untested code for clarity.)

public function remove<T:FlxObject>(Object:T):T {
    if (_registeredObjects == null || _registeredObjects.length == 0) { return Object; }
    for (reg in _registeredObjects) {
        if (reg.object == Object) {
            reg.destroy();
            _registeredObjects.remove(reg);
            break;
        }
    }
    return Object;
}

public function removeAll():Void {
    if (_registeredObjects == null || _registeredObjects.length == 0) { return; }
    for (reg in _registeredObjects) { reg.destroy(); }
    _registeredObjects = [];
}

// . . .

public function destroy():Void {
    object = null; sprite = null;
    onMouseDown = onMouseUp = onMouseOver = onMouseOut = null;
}

Just something to consider. Although, it sounds like there might be other parts of the project that could be improved with the same refactoring, which would be beyond the scope of the issue you are fixing.

Edit: Added a break to the remove() function in my example. After all, it is Bad™ to continue iteration on a collection that has been modified. Plus, there's no need to continue iteration once we've found the element we want to remove.

[MSGhero]

I agree, there are plenty of places where removal logic should be changed. You should make a new issue for it 😁

[MSGhero]

@seraku24 Have you actually tried this fix yet? It properly removes all registered objects, but I don't have your code to see if that was causing your specific issue.

[seraku24]

@MSGhero, I did not file the original issue, #1986, so I cannot comment on that aspect. I just pointed out the computational complexity—yes, I am quite guilty of premature optimization here—and posted a quick sketch of an alternative.

[MSGhero]

Oh crap I mixed up my tabs

[MSGhero]

This PR is good to go, fixes #1986 according to OP of that thread

[Gama11]

I didn't really doubt that it fixes the issue (the test case from the issue doesn't crash anymore), I just didn't think your explanation of why it fixes it makes sense.

[Beeblerox]

i like the solution proposed by @seraku24 at #1992
i don't like the thing that you have to create copy of array to clear it.

[MSGhero]

@Beeblerox yeah, #2005 solves the larger problem. Close this and leave it to that issue?

@Gama11 you're right, I'm not sure why mouseOut was being called if the array containing it gets erased ¯\_(ツ)_/¯ I guess you'd have to step through and see what happens.