QGraphicsSceneFindItemBspTreeVisitor::visit crashes due to an obsolete PaintEvent after QGraphicsScene::removeItem

mbruel

well it's already quite a simple compilable program. You can find a zip of the project here https://github.com/mbruel/testSceneMgr/archive/master.zip
I don't know how I could reproduce it otherwise....
it's just a few classes with no more than 50 lines of code each.
With this project, the crash is reproducible every time on both Windows and Linux.
You just need to launch it, move the link, delete one Element, then delete the link and normally it crashed.

mbruel

Here is a video on how to reproduce it the crash in 5 seconds:
https://sendvid.com/4p5hbt10

josephdp

Hi @mbruel

Looking into your code, you have a circular dependency for link and element classes which is a bad design.

class Element
{
...
QSet<LinkElement *> _links;
}

class LinkElement : public Element
{
...
QSet<Element*> _elements;
}

You need to have a class that manages both link and element (dependency inversion principle).
Same also with Element and GraphicItem classes.

This way you have a clear view of who's managing who and who's knowledgeable with who.

mbruel

@josephdp
Hi Joseph,
In my project I do use intermediate GraphicMangers class between the Element and the Items.
But I still have some reference like on this exemple and it is made by design.
Here is the UML diagram of the example I made. http://postimg.org/image/3zso2igrd/
It is kind of clear:

Element owns a QGraphicsItem that is its graphical representation. (Element is in charge of the deletion of the QGraphicsItem)
LinkElement inherit from Element but is more complex and hold a set _elements of pointers that it is linking (no ownership). It has methods to add and remove elements from this set. Its inherited _item is more complex than for a regular Element, instead of using a ComplexItem it will be a LinkNMItem that inherit from LinkItem.
LinkItem inherit from QGraphicsItem and redefine the mouse events and also the paint. It has a set of pointer on the QGraphicsItems of the Elements that the LinkElement is linking. Also no ownership. There are methods to add/remove those items.

Anyway the crash doesn't come from those "circular" dependencies. It comes from the QGraphicsScene/QGraphicsView.
Doing some tests I can see the QGraphicsScene::removeItem works in the term that QGraphicScene::items().count() decreases BUT for some reason somewhere in QT stack, the item is still in use and the scene/view is not refreshed properly. I need to move another simplier item (one that has a boundingRect with fix size and/or that is not redefining the mouseMoveEvent) in order to have my item really removed from the scene.
I don't understand why... and it is a problem cause I'd like to destroy the item.

If I comment out the deletion of _item in the destructor of Element:

Element::~Element()
{
    for (LinkElement * link : _links)
    {
        link->removeElement(this);
    }
//    delete _item;
}

I then have a memory leak and we can see that the QGraphicsScene::removeItem fails to update the GraphicsView if I delete my link in second.
Cf this video: https://sendvid.com/nagutw1l
The link is still present on the scene until I move other Elements and then disappear at one point.
QT stack holds a pointer on it.
My crash is due to the fact I'm destroying the linkItem although QT still may use it, it even redraws it...

I need a way to make sure it is properly removed and won't be used anymore.

PS: I've changed a little bit the mouse event handlers to properly use or discard the events and it doesn't change the behaviour.

void LinkItem::mouseMoveEvent(QGraphicsSceneMouseEvent *event)
{
    if (_isMoving)
    {
        _movablePoint = event->scenePos();
        update();
        event->accept();
    }
    else
        event->ignore();
}

void LinkItem::mouseReleaseEvent(QGraphicsSceneMouseEvent *event)
{
    QGraphicsItem::mouseReleaseEvent(event);
    if (_isMoving)
    {
        _movablePoint = event->scenePos();
        _isMoving = false;
        _element->setPos(_movablePoint);
        prepareGeometryChange();
    }
}

PS2: I've found a way to avoid this behaviour, it is by setting to true the flag to ignore transformations on the item:

 _item->setFlag(QGraphicsItem::ItemIgnoresTransformations, true);

Unfortunately on my main project I use zoom features that uses transformation so I can't use this "trick"

mbruel

Wow... Finally I think I've found a way to avoid the crash \o/
I've read in some places like here http://stackoverflow.com/questions/38458830/crash-after-qgraphicssceneremoveitem-with-custom-item-class or here: http://www.qtcentre.org/archive/index.php/t-33730.html
that it could be an issue that prepareGeometryChange wasn't called.
But I'm calling it all the time I'm changing the size of my item.
I guess QT gets confused cause this happens often: on mouse events...
Anyway reading about BSP tree and continuing to search I found this link: http://tech-artists.org/forum/showthread.php?3229-Qt-Properly-removing-qGraphicItems
where it suggests to not use BSP indexing!

_scene->setItemIndexMethod(QGraphicsScene::ItemIndexMethod::NoIndex);

coooool!
I'll try it tomorrow to see if that solve the issue on my main project but I'm quite confident it will...

mbruel

Yep, problem solved!
I don't need BSP indexing on my scene. I think this should be a bit more explained in the the head of the documentation of the QGraphicsScene.
http://doc.qt.io/qt-4.8/qgraphicsscene.html#ItemIndexMethod-enum

- QGraphicsScene::BspTreeIndex: A Binary Space Partitioning tree is applied. All QGraphicsScene's item location algorithms are of an order close to logarithmic complexity, by making use of binary search. Adding, moving and removing items is logarithmic. This approach is best for static scenes (i.e., scenes where most items do not move).

- QGraphicsScene::NoIndex: No index is applied. Item location is of linear complexity, as all items on the scene are searched. Adding, moving and removing items, however, is done in constant time. This approach is ideal for dynamic scenes, where many items are added, moved or removed continuously.

But I still find weird that QT keeps a pointer in the BSP indexing Tree on an QGraphicsItem removed from the scene... It's dangerous as the user should be free to delete that QGraphicsItem.
For me, the BSP tree should do all its last treatment before the end of QGraphicsScene::removeItem...

SGaist

Thanks for sharing your findings !

Looks like you may have unearthed something though, maybe report it ?

mbruel

yes, I'm going to share this thread on the QT defect I mentioned before. It was open end of 2012, investigated last year but still not fixed.
it may help to provide them an easy way to reproduce the crash.

aziesemer

I'm having this exact same problem (also described in https://bugreports.qt.io/browse/QTBUG-18021).

I always call prepareGeometryChange before any geometry change, so I have no idea what the problem is.

Some suggested to call prepareGeometryChange before removing the item from the scene but it didn't fix the issue.

Removing BSP indexing solved but is not a good option for me.

cSielaff

Replying here, even though this thread is quite old ... I also ran into the same issue using PySide2. Disabling BSP indexing does indeed work for me, but is a sub-optimal solution because the scene that I'm working with can get arbitrarily large. I also tried to call update prepareGeometryChange before removing the item, and while that did seem to work for a while, the error re-appeared just a few weeks later.

What worked for me (so far) is manually removing all child items before removing the item itself...
To that end, I am overwriting the QGraphicsScene::removeItem method in Python:

class GraphicsScene(QtWidgets.QGraphicsScene):
    def removeItem(self, item: QtWidgets.QGraphicsItem) -> None:
        for child_item in item.childItems():
            super().removeItem(child_item)
        super().removeItem(item)

Note that this will not quite work the same in C++ because QGraphicsScene::removeItem is not a virtual method, so you will probably have to add your own method removeItemSafely or whatever.

Disclaimer: Other methods have worked for me as well ... until they didn't. I have not seen a crash in QGraphicsSceneFindItemBspTreeVisitor::visit since introducing this workaround, but that does not mean that this is actually the solution. Use at your own risk.