Question about RS_EntityContainer

void RS_EntityContainer::reparent(RS_EntityContainer *parent) {
    RS_Entity::reparent(parent);

    // All sub-entities:

    for (auto e: std::as_const(entities)) {
        e->reparent(parent);
    }
}

Why on reparent of entity container, all children are reparented as well? They are still children of its parent. Isn't it?

This effectively flatten the entity tree and breaks some things - for example the line WidthByBlock which after the reparent is determined by the new parent.

well, that's a good question indeed...

The only thing I can imagine for such implementation - is something as following:

1) container is created
2) entities are added into it (and they may be either without parent or have old parent)
3) everything is reparented to the new parent.

Yet even with such scenario - it's more natural to force children be reparented to the container itself, not to the parent of the container...

The current implementation flatten the hierarchy, and I can't say right now what is specific reason for this.
I think for proper addressing your question, it is necessary to revise all methods that deals with hierarchical child->parent logic.

I suspect that some subtle side effects may occur there if this implementation is changed, so this issue definitely requires deeper investigations.

Hm, now I think that it is actually a bug.  Even if made on purpose, such algorithm actually breaks the entities tree structure.

Our containers can have two roles, and it might be a good thing to separate the features to separate classes:

1, the container is the owner of containing entities;
2, the container is only a collection, but not owning the entities.

When reparenting, reparenting the contains meaning overriding the case 1, and enforcing for case 2.

sand1024 wrote

well, that's a good question indeed...

The only thing I can imagine for such implementation - is something as following:

1) container is created
2) entities are added into it (and they may be either without parent or have old parent)
3) everything is reparented to the new parent.

Yet even with such scenario - it's more natural to force children be reparented to the container itself, not to the parent of the container...

The current implementation flatten the hierarchy, and I can't say right now what is specific reason for this.
I think for proper addressing your question, it is necessary to revise all methods that deals with hierarchical child->parent logic.

I suspect that some subtle side effects may occur there if this implementation is changed, so this issue definitely requires deeper investigations.

dxli wrote

When reparenting, reparenting the contains meaning overriding the case 1, and enforcing for case 2.

The reparenting does not affects the ownership at all. It is simply a back pointer in the tree. The forward traversing of the tree is by following items from the QList entities, the back traversing is by following the parent fields.

So, should reparenting break the back links of the tree?

We may have to redesign the undo/redo and ownership.

1. Undo/redo should be able to do garbage collection (currently, all deleted entities stay, even if not reachable anymore);
2. Clear ownership, for example, requiring each entity to have a unique owning container.

johnfound wrote

dxli wrote

When reparenting, reparenting the contains meaning overriding the case 1, and enforcing for case 2.

The reparenting does not affects the ownership at all. It is simply a back pointer in the tree. The forward traversing of the tree is by following items from the QList entities, the back traversing is by following the parent fields.

So, should reparenting break the back links of the tree?

dxli wrote

We may have to redesign the undo/redo and ownership.

1. Undo/redo should be able to do garbage collection (currently, all deleted entities stay, even if not reachable anymore);
2. Clear ownership, for example, requiring each entity to have a unique owning container.

Great, but I still can't see the relation with the reparenting. These are totally different things.

BTW, I simply removed the reparent method from RS_EntityContainer and some rendering issues disappeared. Besides the fixed rendering bug, no negative issues.

Need to verify with all existing usages of this method.

johnfound wrote

dxli wrote

We may have to redesign the undo/redo and ownership.

1. Undo/redo should be able to do garbage collection (currently, all deleted entities stay, even if not reachable anymore);
2. Clear ownership, for example, requiring each entity to have a unique owning container.

Great, but I still can't see the relation with the reparenting. These are totally different things.

BTW, I simply removed the reparent method from RS_EntityContainer and some rendering issues disappeared. Besides the fixed rendering bug, no negative issues.

Ah, yes, verifying, also notice, that RS_Entity::reparent is absolutely the same as RS_Entity::setParent;

    virtual void reparent(RS_EntityContainer *parent){
        this->parent = parent;
    }

void setParent(RS_EntityContainer *p){
        parent = p;
    }

The difference is clearly polymorphism. If you want shallow reparenting, use setparent(), if you want per class behaviors, use the polymorphic version

johnfound wrote

Ah, yes, verifying, also notice, that RS_Entity::reparent is absolutely the same as RS_Entity::setParent;

    virtual void reparent(RS_EntityContainer *parent){
        this->parent = parent;
    }

void setParent(RS_EntityContainer *p){
        parent = p;
    }

Well, please, don't tell general tales.

The "virtual" reparent is overridden only in RS_EntityContainer and wrongly!

And there can't be any correct logic, other than "parent = p". Setting the childrens parent to the new parent is wrong, regardless of the rationale staying behind it. BTW, I suspect that the RS_EntityContainer is a result of copy-paste programming. Compare yourself:

void RS_EntityContainer::reparent(RS_EntityContainer *parent) {
    RS_Entity::reparent(parent);

    // All sub-entities:

    for (auto e: std::as_const(entities)) {
        e->reparent(parent);
    }
}

void RS_EntityContainer::setVisible(bool v) {
    RS_Entity::setVisible(v);

    // All sub-entities:

    for (auto e: std::as_const(entities)) {
        e->setVisible(v);
    }
}