Members of the KDE Community are recommended to subscribe to the kde-community mailing list at https://mail.kde.org/mailman/listinfo/kde-community to allow them to participate in important discussions and receive other important announcements

Commit 36a4a477 authored by Dmitry Kazakov's avatar Dmitry Kazakov

Fixed a lot of double-rendering problems in Flake

The access to the shapes RTree was not consistent. Some shapes were
**never** officially added to the tree and resulted in being added
using the spontaneous update calls. It resulted them in being added
to the tree multiple times, which called the double rendering with all
the consequences.

This patch also adds two sanity checks to KoRTree to catch such
problems automatically in the future.
parent f9e80450
......@@ -31,6 +31,7 @@
#include <QVarLengthArray>
#include <QDebug>
#include "kis_assert.h"
// #define CALLIGRA_RTREE_DEBUG
#ifdef CALLIGRA_RTREE_DEBUG
......@@ -364,6 +365,9 @@ KoRTree<T>::~KoRTree()
template <typename T>
void KoRTree<T>::insert(const QRectF& bb, const T& data)
{
// check if the shape is not already registered
KIS_SAFE_ASSERT_RECOVER_NOOP(!m_leafMap[data]);
insertHelper(bb, data, LeafNode::dataIdCounter++);
}
......@@ -436,13 +440,21 @@ void KoRTree<T>::remove(const T&data)
{
//qDebug() << "KoRTree remove";
LeafNode * leaf = m_leafMap[data];
if (leaf == 0) {
qWarning() << "KoRTree<T>::remove( const T&data) data not found";
return;
}
// Trying to remove unexistent leaf. Most probably, this leaf hasn't been added
// to the shape manager correctly
KIS_SAFE_ASSERT_RECOVER_RETURN(leaf);
m_leafMap.remove(data);
leaf->remove(data);
/**
* WARNING: after calling condenseTree() the temporary enters an inconsistent state!
* m_leafMap still points to the nodes now stored in 'reinsert' list, although
* they are not a part of the hierarchy. This state does not cause any use
* visible changes, but should be considered while implementing sanity checks.
*/
QVector<Node *> reinsert;
condenseTree(leaf, reinsert);
......@@ -708,6 +720,14 @@ void KoRTree<T>::condenseTree(Node *node, QVector<Node*> & reinsert)
//qDebug() << " remove node";
parent->remove(node->place());
reinsert.push_back(node);
/**
* WARNING: here we leave the tree in an inconsistent state! 'reinsert'
* nodes may still be kept in m_leafMap structure, but we will
* *not* remove them for the efficiency reasons. They are guarenteed
* to be readded in remove().
*/
} else {
//qDebug() << " update BB parent is root" << parent->isRoot();
parent->setChildBoundingBox(node->place(), node->boundingBox());
......
......@@ -161,7 +161,6 @@ KoShapePrivate::~KoShapePrivate()
parent->removeShape(q);
Q_FOREACH (KoShapeManager *manager, shapeManagers) {
manager->remove(q);
manager->removeAdditional(q);
}
if (shadow && !shadow->deref())
......
......@@ -203,7 +203,8 @@ void KoShapeContainer::ShapeInterface::addShape(KoShape *shape)
{
KoShapeContainerPrivate * const d = q->d_func();
Q_ASSERT(shape);
KIS_SAFE_ASSERT_RECOVER_RETURN(shape);
if (shape->parent() == q && q->shapes().contains(shape)) {
return;
}
......@@ -218,6 +219,7 @@ void KoShapeContainer::ShapeInterface::addShape(KoShape *shape)
}
d->model->add(shape);
d->model->shapeHasBeenAddedToHierarchy(shape, q);
}
void KoShapeContainer::ShapeInterface::removeShape(KoShape *shape)
......@@ -228,6 +230,7 @@ void KoShapeContainer::ShapeInterface::removeShape(KoShape *shape)
KIS_SAFE_ASSERT_RECOVER_RETURN(d->model);
KIS_SAFE_ASSERT_RECOVER_RETURN(d->model->shapes().contains(shape));
d->model->shapeToBeRemovedFromHierarchy(shape, q);
d->model->remove(shape);
KoShapeContainer *grandparent = q->parent();
......
......@@ -63,6 +63,22 @@ void KoShapeContainerModel::childChanged(KoShape *child, KoShape::ChangeType typ
}
}
void KoShapeContainerModel::shapeHasBeenAddedToHierarchy(KoShape *shape, KoShapeContainer *addedToSubtree)
{
KoShapeContainer *parent = addedToSubtree->parent();
if (parent) {
parent->model()->shapeHasBeenAddedToHierarchy(shape, parent);
}
}
void KoShapeContainerModel::shapeToBeRemovedFromHierarchy(KoShape *shape, KoShapeContainer *removedFromSubtree)
{
KoShapeContainer *parent = removedFromSubtree->parent();
if (parent) {
parent->model()->shapeToBeRemovedFromHierarchy(shape, parent);
}
}
KoShapeContainerModel::KoShapeContainerModel(const KoShapeContainerModel &rhs)
{
Q_UNUSED(rhs);
......
......@@ -171,6 +171,9 @@ public:
*/
virtual void childChanged(KoShape *shape, KoShape::ChangeType type);
virtual void shapeHasBeenAddedToHierarchy(KoShape *shape, KoShapeContainer *addedToSubtree);
virtual void shapeToBeRemovedFromHierarchy(KoShape *shape, KoShapeContainer *removedFromSubtree);
protected:
KoShapeContainerModel(const KoShapeContainerModel &rhs);
};
......
......@@ -51,6 +51,10 @@
#include "kis_painting_tweaks.h"
bool KoShapeManager::Private::shapeUsedInRenderingTree(KoShape *shape)
{
return !dynamic_cast<KoShapeGroup*>(shape) && !dynamic_cast<KoShapeLayer*>(shape);
}
void KoShapeManager::Private::updateTree()
{
......@@ -66,14 +70,17 @@ void KoShapeManager::Private::updateTree()
}
foreach (KoShape *shape, aggregate4update) {
if (!shapeUsedInRenderingTree(shape)) continue;
tree.remove(shape);
QRectF br(shape->boundingRect());
tree.insert(br, shape);
}
// do it again to see which shapes we intersect with _after_ moving.
foreach (KoShape *shape, aggregate4update)
foreach (KoShape *shape, aggregate4update) {
detector.detect(tree, shape, shapeIndexesBeforeUpdate[shape]);
}
aggregate4update.clear();
shapeIndexesBeforeUpdate.clear();
......@@ -153,7 +160,7 @@ void KoShapeManager::addShape(KoShape *shape, Repaint repaint)
return;
shape->priv()->addShapeManager(this);
d->shapes.append(shape);
if (! dynamic_cast<KoShapeGroup*>(shape) && ! dynamic_cast<KoShapeLayer*>(shape)) {
if (d->shapeUsedInRenderingTree(shape)) {
QRectF br(shape->boundingRect());
d->tree.insert(br, shape);
}
......@@ -175,17 +182,6 @@ void KoShapeManager::addShape(KoShape *shape, Repaint repaint)
detector.fireSignals();
}
void KoShapeManager::addAdditional(KoShape *shape)
{
if (shape) {
if (d->additionalShapes.contains(shape)) {
return;
}
shape->priv()->addShapeManager(this);
d->additionalShapes.append(shape);
}
}
void KoShapeManager::remove(KoShape *shape)
{
Private::DetectCollision detector;
......@@ -196,7 +192,9 @@ void KoShapeManager::remove(KoShape *shape)
shape->priv()->removeShapeManager(this);
d->selection->deselect(shape);
d->aggregate4update.remove(shape);
d->tree.remove(shape);
if (d->shapeUsedInRenderingTree(shape)) {
d->tree.remove(shape);
}
d->shapes.removeAll(shape);
// remove the children of a KoShapeContainer
......@@ -212,14 +210,6 @@ void KoShapeManager::remove(KoShape *shape)
shapeRemoved(shape);
}
void KoShapeManager::removeAdditional(KoShape *shape)
{
if (shape) {
shape->priv()->removeShapeManager(this);
d->additionalShapes.removeAll(shape);
}
}
void KoShapeManager::paint(QPainter &painter, const KoViewConverter &converter, bool forPrint)
{
d->updateTree();
......
......@@ -99,12 +99,6 @@ public Q_SLOTS:
*/
void addShape(KoShape *shape, KoShapeManager::Repaint repaint = PaintShapeOnAdd);
/**
* Add an additional shape to the manager.
*
* For additional shapes only updates are handled
*/
void addAdditional(KoShape *shape);
/**
* Remove a KoShape from this manager
......@@ -112,13 +106,6 @@ public Q_SLOTS:
*/
void remove(KoShape *shape);
/**
* Remove an additional shape
*
* For additional shapes only updates are handled
*/
void removeAdditional(KoShape *shape);
public:
/// return the selection shapes for this shapeManager
KoSelection *selection() const;
......
......@@ -55,6 +55,12 @@ public:
*/
void updateTree();
/**
* Returns whether the shape should be added to the RTree for collision and ROI
* detection.
*/
bool shapeUsedInRenderingTree(KoShape *shape);
/**
* Recursively paints the given group shape to the specified painter
* This is needed for filter effects on group shapes where the filter effect
......
......@@ -76,6 +76,34 @@ class MockGroup : public KoShapeGroup
class KoToolProxy;
class MockShapeController : public KoShapeBasedDocumentBase
{
public:
void addShape(KoShape* shape) {
m_shapes.insert(shape);
if (m_shapeManager) {
m_shapeManager->addShape(shape);
}
}
void removeShape(KoShape* shape) {
m_shapes.remove(shape);
if (m_shapeManager) {
m_shapeManager->remove(shape);
}
}
bool contains(KoShape* shape) {
return m_shapes.contains(shape);
}
void setShapeManager(KoShapeManager *shapeManager) {
m_shapeManager = shapeManager;
}
private:
QSet<KoShape * > m_shapes;
KoShapeManager *m_shapeManager = 0;
};
class MockCanvas : public KoCanvasBase
{
public:
......@@ -84,6 +112,9 @@ public:
m_shapeManager(new KoShapeManager(this)),
m_selectedShapesProxy(new KoSelectedShapesProxySimple(m_shapeManager.data()))
{
if (MockShapeController *controller = dynamic_cast<MockShapeController*>(aKoShapeBasedDocumentBase)) {
controller->setShapeManager(m_shapeManager.data());
}
}
~MockCanvas() {}
......@@ -134,22 +165,6 @@ public:
qreal m_vert;
};
class MockShapeController : public KoShapeBasedDocumentBase
{
public:
void addShape(KoShape* shape) {
m_shapes.insert(shape);
}
void removeShape(KoShape* shape) {
m_shapes.remove(shape);
}
bool contains(KoShape* shape) {
return m_shapes.contains(shape);
}
private:
QSet<KoShape * > m_shapes;
};
class MockContainerModel : public KoShapeContainerModel
{
public:
......
......@@ -252,5 +252,74 @@ void TestShapePainting::testPaintOrder()
delete bottom;
delete root;
}
#include <kundo2command.h>
#include <KoShapeController.h>
#include <KoShapeGroupCommand.h>
#include <KoShapeUngroupCommand.h>
#include "kis_debug.h"
void TestShapePainting::testGroupUngroup()
{
MockShape *shape1 = new MockShape();
MockShape *shape2 = new MockShape();
shape1->setName("shape1");
shape2->setName("shape2");
QList<KoShape*> groupedShapes = {shape1, shape2};
MockShapeController controller;
MockCanvas canvas(&controller);
KoShapeManager *manager = canvas.shapeManager();
controller.addShape(shape1);
controller.addShape(shape2);
QImage image(100, 100, QImage::Format_Mono);
QPainter painter(&image);
painter.setClipRect(image.rect());
KoViewConverter vc;
KoShapeGroup *group = 0;
for (int i = 0; i < 3; i++) {
{
group = new KoShapeGroup();
group->setName("group");
KUndo2Command groupingCommand;
canvas.shapeController()->addShapeDirect(group, &groupingCommand);
new KoShapeGroupCommand(group, groupedShapes, false, true, true, &groupingCommand);
groupingCommand.redo();
manager->paint(painter, vc, false);
QCOMPARE(shape1->paintedCount, 2 * i + 1);
QCOMPARE(shape2->paintedCount, 2 * i + 1);
QCOMPARE(manager->shapes().size(), 3);
}
{
KUndo2Command ungroupingCommand;
new KoShapeUngroupCommand(group, group->shapes(), QList<KoShape*>(), &ungroupingCommand);
canvas.shapeController()->removeShape(group, &ungroupingCommand);
ungroupingCommand.redo();
manager->paint(painter, vc, false);
QCOMPARE(shape1->paintedCount, 2 * i + 2);
QCOMPARE(shape2->paintedCount, 2 * i + 2);
QCOMPARE(manager->shapes().size(), 2);
group = 0;
}
}
}
QTEST_MAIN(TestShapePainting)
......@@ -30,6 +30,7 @@ private Q_SLOTS:
void testPaintShape();
void testPaintHiddenShape();
void testPaintOrder();
void testGroupUngroup();
};
#endif
......@@ -98,14 +98,22 @@ public:
void add(KoShape *child) override {
SimpleShapeContainerModel::add(child);
q->shapeManager()->addShape(child);
}
void remove(KoShape *child) override {
q->shapeManager()->remove(child);
SimpleShapeContainerModel::remove(child);
}
void shapeHasBeenAddedToHierarchy(KoShape *shape, KoShapeContainer *addedToSubtree) override {
q->shapeManager()->addShape(shape);
SimpleShapeContainerModel::shapeHasBeenAddedToHierarchy(shape, addedToSubtree);
}
void shapeToBeRemovedFromHierarchy(KoShape *shape, KoShapeContainer *removedFromSubtree) override {
q->shapeManager()->remove(shape);
SimpleShapeContainerModel::shapeToBeRemovedFromHierarchy(shape, removedFromSubtree);
}
private:
KisShapeLayer *q;
};
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment