-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deleting object does not delete keyframes in Graph #176
Comments
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. |
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 MaurycyLiebner#38 the graph includes all animators added from any box and as the issue says it becomes cluttered. With MaurycyLiebner@071675a 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.
Remove the "view all"/"view selected" option and feature, problem fixed.
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... :) |
does not this design pattern create a big problem of file bloating will lots of unused / hidden stuff inside ?
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 |
It's not saved, they only exists as long as the canvas is open (in memory).
Yeah, but that's a major refactor :) We need a solution before 0.9.6 release. |
there is an option 3. just ignore this bug and fix it properly in 1.0 + |
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. |
sounds like a plan i do not like hacky solutions but also dot think removing feature is a good idea |
Solution 2We introduce When box is removed: We add When box is added: We add and we add
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. |
looks good to me (don't listen to me on code advice :D). If you push out build i will test |
Just brainstorming, it's also more about logic than code :) Will try to push out a build later tonight, currently at work. |
it sounds like logical solution |
This is a proposed solution for issue #176. Introduces a new "isGraphSelected" function for boxes and "prp_isParentBoxGraphSelected" for properties. This function is used by the graph to figure out which animators are usable or not, the state is controlled by the canvas. if "isGraphSelected" is false then the box should be considered removed and the animator connected to the box should be ignored, we do not remove it since that breaks undo.
keyframes are still there
friction_qUzhrxUKoc.mp4
keyframe should be deleted with the object
The text was updated successfully, but these errors were encountered: