Bugfix/uum 141406 fixing action overlay behavior#672
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR introduces static event subscriptions to PreviewActionManager.delayedPreviewChanged across several menu action settings panels. However, because these anonymous subscribers are not unregistered when the panels are destroyed, they will cause memory leaks and multiple handler invocations.
Summary of Findings
- Static Event Memory Leaks (High Importance): Multiple menu action classes (
BevelEdges,ExtrudeEdges,ExtrudeFaces,OffsetElements,SubdivideEdges,WeldVertices, andGrowSelection) register event handlers to the static eventPreviewActionManager.delayedPreviewChangedwithout a mechanism to unsubscribe. These subscriptions should be cleaned up using theDetachFromPanelEventcallback on the root visual elements.
🤖 Helpful? 👍/👎
| } | ||
| }); | ||
|
|
||
| PreviewActionManager.delayedPreviewChanged += () => |
There was a problem hiding this comment.
Subscribing to a static event (PreviewActionManager.delayedPreviewChanged) using an anonymous delegate inside CreateSettingsContent will cause a memory leak. Because static events hold strong references to their subscribers, the floatField (and the entire parent VisualElement tree) will be kept alive in memory indefinitely even after the settings content is destroyed.
To prevent this, you should unsubscribe from the event when the visual element is detached. Have you considered registering a callback on the DetachFromPanelEvent to clean up the subscription?
Suggested change (apply manually):
System.Action onDelayedPreviewChanged = () =>
{
floatField.isDelayed = PreviewActionManager.delayedPreview;
};
PreviewActionManager.delayedPreviewChanged += onDelayedPreviewChanged;
root.RegisterCallback<DetachFromPanelEvent>(evt =>
{
PreviewActionManager.delayedPreviewChanged -= onDelayedPreviewChanged;
});🤖 Helpful? 👍/👎 by guardian
| floatField.tooltip = "Extrude Amount determines how far an edge will be moved along it's normal when extruding. This value can be negative."; | ||
| floatField.SetValueWithoutNotify(m_ExtrudeEdgeDistance); | ||
| floatField.RegisterCallback<ChangeEvent<float>>(OnExtrudeChanged); | ||
| PreviewActionManager.delayedPreviewChanged += () => |
There was a problem hiding this comment.
Subscribing to a static event (PreviewActionManager.delayedPreviewChanged) using an anonymous delegate inside CreateSettingsContent causes a memory leak because the static event holds onto the visual elements even after they are discarded.
You can avoid this by unsubscribing from the event when the element is detached from the panel.
Suggested change (apply manually):
System.Action onDelayedPreviewChanged = () =>
{
floatField.isDelayed = PreviewActionManager.delayedPreview;
};
PreviewActionManager.delayedPreviewChanged += onDelayedPreviewChanged;
root.RegisterCallback<DetachFromPanelEvent>(evt =>
{
PreviewActionManager.delayedPreviewChanged -= onDelayedPreviewChanged;
});🤖 Helpful? 👍/👎 by guardian
| }); | ||
| root.Add(distanceField); | ||
|
|
||
| PreviewActionManager.delayedPreviewChanged += () => |
There was a problem hiding this comment.
Subscribing to a static event (PreviewActionManager.delayedPreviewChanged) using an anonymous delegate inside CreateSettingsContent causes a memory leak because the static event holds onto the visual elements even after they are discarded.
You can avoid this by unsubscribing from the event when the element is detached from the panel.
Suggested change (apply manually):
System.Action onDelayedPreviewChanged = () =>
{
distanceField.isDelayed = PreviewActionManager.delayedPreview;
};
PreviewActionManager.delayedPreviewChanged += onDelayedPreviewChanged;
root.RegisterCallback<DetachFromPanelEvent>(evt =>
{
PreviewActionManager.delayedPreviewChanged -= onDelayedPreviewChanged;
});🤖 Helpful? 👍/👎 by guardian
| PreviewActionManager.UpdatePreview(); | ||
| }); | ||
|
|
||
| PreviewActionManager.delayedPreviewChanged += () => |
There was a problem hiding this comment.
Subscribing to a static event (PreviewActionManager.delayedPreviewChanged) using an anonymous delegate inside CreateSettingsContent causes a memory leak because the static event holds onto the visual elements even after they are discarded.
You can avoid this by unsubscribing from the event when the element is detached from the panel.
Suggested change (apply manually):
System.Action onDelayedPreviewChanged = () =>
{
distField.Query<FloatField>().ForEach(ff => ff.isDelayed = PreviewActionManager.delayedPreview);
};
PreviewActionManager.delayedPreviewChanged += onDelayedPreviewChanged;
root.RegisterCallback<DetachFromPanelEvent>(evt =>
{
PreviewActionManager.delayedPreviewChanged -= onDelayedPreviewChanged;
});🤖 Helpful? 👍/👎 by guardian
| m_SubdivCount.tooltip = tooltip; | ||
| m_SubdivCount.RegisterCallback<ChangeEvent<int>>(OnCountChanged); | ||
| m_SubdivCount.style.width = 40; | ||
| PreviewActionManager.delayedPreviewChanged += () => |
There was a problem hiding this comment.
Subscribing to a static event (PreviewActionManager.delayedPreviewChanged) using an anonymous delegate inside CreateSettingsContent causes a memory leak because the static event holds onto the visual elements even after they are discarded.
You can avoid this by unsubscribing from the event when the element is detached from the panel.
Suggested change (apply manually):
System.Action onDelayedPreviewChanged = () =>
{
m_SubdivCount.isDelayed = PreviewActionManager.delayedPreview;
};
PreviewActionManager.delayedPreviewChanged += onDelayedPreviewChanged;
root.RegisterCallback<DetachFromPanelEvent>(evt =>
{
PreviewActionManager.delayedPreviewChanged -= onDelayedPreviewChanged;
});🤖 Helpful? 👍/👎 by guardian
| m_WeldDistance.SetValue(evt.newValue); | ||
| PreviewActionManager.UpdatePreview(); | ||
| }); | ||
| PreviewActionManager.delayedPreviewChanged += () => |
There was a problem hiding this comment.
Subscribing to a static event (PreviewActionManager.delayedPreviewChanged) using an anonymous delegate inside CreateSettingsContent causes a memory leak because the static event holds onto the visual elements even after they are discarded.
You can avoid this by unsubscribing from the event when the element is detached from the panel.
Suggested change (apply manually):
System.Action onDelayedPreviewChanged = () =>
{
floatField.isDelayed = PreviewActionManager.delayedPreview;
};
PreviewActionManager.delayedPreviewChanged += onDelayedPreviewChanged;
root.RegisterCallback<DetachFromPanelEvent>(evt =>
{
PreviewActionManager.delayedPreviewChanged -= onDelayedPreviewChanged;
});🤖 Helpful? 👍/👎 by guardian
| PreviewActionManager.UpdatePreview(); | ||
| }); | ||
|
|
||
| PreviewActionManager.delayedPreviewChanged += () => |
There was a problem hiding this comment.
Subscribing to a static event (PreviewActionManager.delayedPreviewChanged) using an anonymous delegate inside CreateSettingsContent causes a memory leak because the static event holds onto the visual elements even after they are discarded.
You can avoid this by unsubscribing from the event when the element is detached from the panel.
Suggested change (apply manually):
System.Action onDelayedPreviewChanged = () =>
{
floatField.isDelayed = PreviewActionManager.delayedPreview;
};
PreviewActionManager.delayedPreviewChanged += onDelayedPreviewChanged;
root.RegisterCallback<DetachFromPanelEvent>(evt =>
{
PreviewActionManager.delayedPreviewChanged -= onDelayedPreviewChanged;
});🤖 Helpful? 👍/👎 by guardian
2 participants
##Purpose of this PR
Jira: UUM-141406 (https://jira.unity3d.com/browse/UUM-141406)
This PR fixes two related issues with ProBuilder action overlays:
triggered, the overlay would not properly refresh or display. The fix adds a workaround that toggles displayed from
false to true to ensure the overlay is properly added/refreshed in the view.
fields (FloatField, IntegerField, Vector3Field) would not dynamically update their isDelayed property. Users had to
restart the action for the setting to take effect.
###Solution:
WeldVertices, GrowSelection) to subscribe to this event and update their field properties dynamically
##Release Notes
Fixed: UUM-141406 - Action overlays now display correctly when triggering a new action while an overlay is already shown in the SceneView.
##Functional Testing status
Manual Testing:
Edges, Weld Vertices, Grow Selection)
Existing Automation:
ProBuilder has existing editor tests for action functionality. The overlay display and UI field behavior changes are
UI-specific and covered by manual testing.
##Performance Testing Status
Performance Impact: Minimal positive impact
The change replaces an inefficient approach (destroying and recreating the entire overlay UI hierarchy) with an
event-based approach that only updates the isDelayed property on existing fields. This reduces allocations and UI
rebuilds when toggling the live preview setting.
No performance testing required - this is a UI behavior fix with a minor efficiency improvement.
##Overall Product Risks
Technical Risk: 1
Isolated UI behavior changes in ProBuilder action overlays. The event-based approach is straightforward, and the
overlay display workaround is localized. Low risk of regression.
Halo Effect: 1
Changes are isolated to ProBuilder's action overlay system and the seven modified geometry actions. Does not affect
other ProBuilder functionality, other packages, or Unity Editor core systems.