Deleting object does not delete keyframes in Graph

[adambelis]

1. Open Fricion and create a rectangle
2. add animation keyframes to any properties
3. enable visibility in graph
4. delete object
5. switch to graph

keyframes are still there

https://github.com/friction2d/friction/assets/50795097/fa9c26a9-aa75-48d4-93b2-fefd049409d0

keyframe should be deleted with the object

[rodlie]

Thanks for detecting this issue.

At the code level graph animators are removed when the object is destroyed. Either the object is not actually destroyed or the destroy signal is never emitted.

EDIT: I can confirm that the object is never deleted, so this has never worked.

[rodlie]

So.... I escaped the rabbit hole.

On the surface this looks like a trivial issue, maybe a simple redraw after the object is removed? No.

The canvas and timeline/graph has a simple concept, any object added is "selected" and any object removed is "unselected" (Some of you may already start to see a problem). Objects are not deleted, they just change state. A "deleted" and "unselected" object will return the same state (weeee).

The graph has something called graph animators, this is the keyframe object(s) for whatever you added to the graph, they have a parent, the "box" (this is your rectangle, path, eclipse, image, video etc).

So, when I delete the box then the graph animators are also removed right? No.

This is a feature (with bugs).

In enve up to https://github.com/MaurycyLiebner/enve/issues/38 the graph includes all animators added from any box and as the issue says it becomes cluttered. With https://github.com/MaurycyLiebner/enve/commit/071675a36e4c90c76240c177f9c684159f609ecc you now have the option ("View only Selected") to show only graph animators from selected boxes (in this context selected means selected, not added/removed... yeah, fun, fun).

The only way to remove a graph animator is when it's destroyed (that won't work!). You can turn on "View only Selected", this will "work" from a user perspective (but it's still there).

So... Solutions?

Yes, there are (at least) two.

1. The easy way

Remove the "view all"/"view selected" option and feature, problem fixed.

2. Add new box state

Introduce a new state for boxes, something like isEnabled/isRemoved whatever that we check before isSelected in the graph update. This is kind of hacky and feels like a workaround for a workaround... :)

[adambelis]

The canvas and timeline/graph has a simple concept, any object added is "selected" and any object removed is "unselected" (Some of you may already start to see a problem). Objects are not deleted, they just change state. A "deleted" and "unselected" object will return the same state (weeee).

does not this design pattern create a big problem of file bloating will lots of unused / hidden stuff inside ?

The easy way Remove the "view all"/"view selected" option and feature, problem fixed.

you know i kind of prosoe hypride of this two features i think current workflow is clumsy . but my solution would not solve this problem :D

[rodlie]

does not this design pattern create a big problem of file bloating will lots of unused / hidden stuff inside ?

It's not saved, they only exists as long as the canvas is open (in memory).

you know i kind of prosoe hypride of this two features i think current workflow is clumsy . but my solution would not solve this problem :D

Yeah, but that's a major refactor :) We need a solution before 0.9.6 release.

[adambelis]

there is an option 3. just ignore this bug and fix it properly in 1.0 +

[rodlie]

Pandora's box is open, can't ignore it now :)

I will create two branches with solution 1 and 2 and we take it from there.

[adambelis]

sounds like a plan i do not like hacky solutions but also dot think removing feature is a good idea

[rodlie]

Solution 2

We introduce prp_isParentBoxRemoved. This will only be used by GraphAnimator.

When box is removed: https://github.com/friction2d/friction/blob/main/src/core/canvasselectedboxesactions.cpp#L460

We add box->setRemoved(true);.

When box is added: https://github.com/friction2d/friction/blob/main/src/core/canvasselectedboxesactions.cpp#L441

We add box->setRemoved(false);.

and we add

    if (animator->prp_isParentBoxRemoved()) {
        graphRemoveViewedAnimator(animator);
        return false;
    }

before https://github.com/friction2d/friction/blob/main/src/app/GUI/graphboxeslist.cpp#L475

This should in theory fix the issue. When a box is deleted we set removed state, when the graph is updated we check the removed state before selected state and remove the graph animator if the box was removed.

[adambelis]

looks good to me (don't listen to me on code advice :D). If you push out build i will test

[rodlie]

Just brainstorming, it's also more about logic than code :)

Will try to push out a build later tonight, currently at work.

[adambelis]

it sounds like logical solution