Skip to content
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

WidgetContainer memory management refacto #287

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

maxbrundev
Copy link
Collaborator

Addressing issue #281 and composing Panels with CallbackQueue in order to allow widgets ownership manipulation before iterate on them.

▶️ The WidgetContainer is now replacing Consider / Unconsider methods for widgets manipulation by transferring the ownership of them. This introduced issues where the plugins was transferring the ownership as the same time we are iterating other them.

▶️ In order to fix this issue the CallbackQueue class has been implemented and now composed Panels, this allows us to handle widget manipulation before Drawing them.

Before the CallbackQueue implementation:

Draw the widgets -> Execute the plugin before Drawing the current widget -> Drag a Treenode -> the plugin call an Actor remove parent method -> Actor send an Event -> We handle the event directly in the Hierarchy Panel and remove the ownership of the current widget -> Crash: we are currently iterating other widget ❌

After the CallbackQueue implementation:

Draw the widgets -> Execute the plugin before Drawing the current widget -> Drag a Treenode -> the plugin call an Actor remove parent method -> Actor send an Event -> the Hierarchy Panel enqueue the event -> Finishing to drawing the widgets -> processing the event before the next draw ✔️

@maxbrundev maxbrundev self-assigned this Dec 9, 2023
Copy link
Owner

@adriengivry adriengivry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about the hierarchy having to be aware of the delayed callback processing system, as this should be the sole responsibility of the WidgetContainer. The editor shouldn't have to care about delaying ownership transfers, as it might be error prone. While it works for this specific case, this will definitely introduce technical debt and might start to bloat the code base the more editor features need to rely on this system.

Also, we already have a system in place in OvEditor to delay actions by X frames (See EditorActions::DelayAction), although as I mentioned, I don't believe the editor should ever have to manage this WidgetContainer limitation by itself.

Ideally the editor code wouldn't change outside of calling TransferOwnership instead of Unconsider/Consider.

Some nit: I feel like TransferOwnership is a bit hard to use as-is, as it's a method that requires an instance of WidgetContainer to be invoked: instance.TransferOwnership(...). Just by reading it, TransferOwnership doesn't tell me what kind of ownership is going to be transferered, nor what's the source and destination. Maybe having a static method like:
WidgetContainer::TransferWidget(widget, from, to) would make more sense? Or we could add it directly to the widget: widget.MoveTo(newParent) (might not be ideal as the Widget shouldn't know about the WidgetContainer). I don't have a concrete solution for this at the moment, but this is something I believe is worth looking into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants