From ac001fb42e63571f1d9b51e439b02ca5846c7a0b Mon Sep 17 00:00:00 2001 From: bd_ Date: Mon, 14 Oct 2024 21:29:51 -0700 Subject: [PATCH] fix: flickering selection outlines (#454) Previously, we swapped between two proxy renderers in order to minimize the cost of building new game objects all the time, and to allow a new pipeline to build in parallel with rendering the prior pipeline. This worked, but caused flickering effects, due to the unity selection system being stateful and remembering the previously selected/highlighted instance ID. Here, we instead have a single primary proxy used for all rendering, and use alternate objects while setting up a shadow proxy pipeline. Closes: #453 --- CHANGELOG.md | 1 + .../Rendering/ProxyObjectCache.cs | 148 +++++++++++------- .../Rendering/ProxyObjectController.cs | 57 ++++--- .../PreviewSystem/Rendering/ProxyPipeline.cs | 13 +- 4 files changed, 135 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6032b2..a7ac339 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added ### Fixed +- [#454] Scene view selection outlines flicker in some cases - [#450] Improved performance when a large number of object change events are generated (e.g. when exiting animation mode) - [#441] Fixed an issue where the preview object pickable status could get out of sync with the original diff --git a/Editor/PreviewSystem/Rendering/ProxyObjectCache.cs b/Editor/PreviewSystem/Rendering/ProxyObjectCache.cs index 69739a9..9317eea 100644 --- a/Editor/PreviewSystem/Rendering/ProxyObjectCache.cs +++ b/Editor/PreviewSystem/Rendering/ProxyObjectCache.cs @@ -17,15 +17,75 @@ public static bool IsProxyObject(GameObject obj) return _proxyObjectInstanceIds.Contains(obj.GetInstanceID()); } - + + public interface IProxyHandle : IDisposable + { + public Renderer PrimaryProxy { get; } + public Renderer GetSetupProxy(); + public void ReturnSetupProxy(Renderer proxy); + } + + private class ProxyHandleImpl : IProxyHandle + { + private readonly ProxyObjectCache _cache; + private readonly Renderer _key; + private readonly Func _createFunc; + private readonly RendererState _state; + + public ProxyHandleImpl(ProxyObjectCache cache, Renderer key, Func createFunc, RendererState state) + { + _cache = cache; + _key = key; + _createFunc = createFunc; + _state = state; + } + + public Renderer PrimaryProxy => _state.PrimaryProxy; + + public Renderer GetSetupProxy() + { + if (_state.InactiveSetupProxy != null) + { + var proxy = _state.InactiveSetupProxy; + proxy.enabled = true; + _state.InactiveSetupProxy = null; + return proxy; + } + + return _createFunc(); + } + + public void ReturnSetupProxy(Renderer proxy) + { + if (_state.InactiveSetupProxy != null || _state.ActivePrimaryCount == 0) + { + DestroyProxy(proxy); + } + else + { + proxy.enabled = false; + _state.InactiveSetupProxy = proxy; + } + } + + public void Dispose() + { + if (--_state.ActivePrimaryCount == 0) + { + _cache.MaybeDisposeProxy(_key); + } + } + } + private class RendererState { - public Renderer InactiveProxy; - public int ActiveProxyCount = 0; + public Renderer PrimaryProxy; + public int ActivePrimaryCount; + + public Renderer InactiveSetupProxy; } private readonly Dictionary _renderers = new(); - private bool _cleanupPending = false; private bool _isRegistered = false; private bool IsRegistered @@ -37,10 +97,12 @@ private bool IsRegistered if (value) { EditorApplication.playModeStateChanged += OnPlayModeStateChanged; + EditorApplication.update += Cleanup; } else { EditorApplication.playModeStateChanged -= OnPlayModeStateChanged; + EditorApplication.update -= Cleanup; } _isRegistered = value; @@ -53,69 +115,26 @@ private void OnPlayModeStateChanged(PlayModeStateChange obj) Dispose(); } - public Renderer GetOrCreate(Renderer original, Func create) + public IProxyHandle GetHandle(Renderer original, Func create) { IsRegistered = true; if (!_renderers.TryGetValue(original, out var state)) { state = new RendererState(); + state.PrimaryProxy = create(); _renderers.Add(original, state); } - Renderer proxy; - if (state.InactiveProxy != null) - { - proxy = state.InactiveProxy; - state.InactiveProxy = null; - state.ActiveProxyCount++; - proxy.enabled = true; - return proxy; - } - - proxy = create(); - if (proxy == null) - { - return null; - } - - state.ActiveProxyCount++; - _proxyObjectInstanceIds.Add(proxy.gameObject.GetInstanceID()); - - return proxy; - } - - public void ReturnProxy(Renderer original, Renderer proxy) - { - IsRegistered = true; - - if (!_renderers.TryGetValue(original, out var state)) - { - DestroyProxy(proxy); - return; - } - - if (!_cleanupPending) - { - EditorApplication.delayCall += Cleanup; - _cleanupPending = true; - } + state.ActivePrimaryCount++; - state.ActiveProxyCount--; - if (state.ActiveProxyCount > 0 && state.InactiveProxy == null) + return new ProxyHandleImpl(this, original, () => { - state.InactiveProxy = proxy; - proxy.enabled = false; - return; - } - - DestroyProxy(proxy); + var newProxy = create(); + _proxyObjectInstanceIds.Add(newProxy.gameObject.GetInstanceID()); - if (state.ActiveProxyCount == 0) - { - DestroyProxy(state.InactiveProxy); - _renderers.Remove(original); - } + return newProxy; + }, state); } private static void DestroyProxy(Renderer proxy) @@ -127,13 +146,23 @@ private static void DestroyProxy(Renderer proxy) Object.DestroyImmediate(gameObject); } + + private void MaybeDisposeProxy(Renderer key) + { + if (_renderers.TryGetValue(key, out var state) && state.ActivePrimaryCount == 0) + { + DestroyProxy(state.PrimaryProxy); + DestroyProxy(state.InactiveSetupProxy); + _renderers.Remove(key); + } + } + private void Cleanup() { - _cleanupPending = false; - foreach (var entry in _renderers.Where(kv => kv.Key == null).ToList()) { - DestroyProxy(entry.Value.InactiveProxy); + DestroyProxy(entry.Value.InactiveSetupProxy); + DestroyProxy(entry.Value.PrimaryProxy); _renderers.Remove(entry.Key); } } @@ -142,7 +171,8 @@ public void Dispose() { foreach (var entry in _renderers) { - DestroyProxy(entry.Value.InactiveProxy); + DestroyProxy(entry.Value.InactiveSetupProxy); + DestroyProxy(entry.Value.PrimaryProxy); } _renderers.Clear(); IsRegistered = false; diff --git a/Editor/PreviewSystem/Rendering/ProxyObjectController.cs b/Editor/PreviewSystem/Rendering/ProxyObjectController.cs index ff7b183..0bb02a1 100644 --- a/Editor/PreviewSystem/Rendering/ProxyObjectController.cs +++ b/Editor/PreviewSystem/Rendering/ProxyObjectController.cs @@ -2,7 +2,6 @@ using System; using System.Linq; -using System.Threading.Tasks; using UnityEditor; using UnityEngine; using UnityEngine.Profiling; @@ -17,9 +16,11 @@ internal class ProxyObjectController : IDisposable { private readonly ProxyObjectCache _cache; private readonly Renderer _originalRenderer; - private Renderer _replacementRenderer; - internal Renderer Renderer => _replacementRenderer; - public bool IsValid => _originalRenderer != null && _replacementRenderer != null; + private bool _setupComplete; + private Renderer _setupProxy, _primaryProxy; + private ProxyObjectCache.IProxyHandle _proxyHandle; + + internal Renderer Renderer => _setupComplete ? _primaryProxy : _setupProxy; internal RenderAspects ChangeFlags; @@ -132,9 +133,9 @@ private void SetupRendererMonitoring(Renderer r) internal bool OnPreFrame() { - if (_replacementRenderer == null || _originalRenderer == null) + if (Renderer == null || _originalRenderer == null) { - if (_replacementRenderer == null) + if (Renderer == null) { Debug.LogWarning("Proxy object was destroyed improperly! Resetting pipeline..."); } @@ -145,7 +146,7 @@ internal bool OnPreFrame() try { - var target = _replacementRenderer; + var target = Renderer; var original = _originalRenderer; if (VisibilityMonitor.Sequence != _lastVisibilityCheck) @@ -176,7 +177,7 @@ internal bool OnPreFrame() { smr = smr_; - var replacementSMR = (SkinnedMeshRenderer)_replacementRenderer; + var replacementSMR = (SkinnedMeshRenderer)Renderer; replacementSMR.sharedMesh = smr_.sharedMesh; replacementSMR.bones = smr_.bones; @@ -186,12 +187,12 @@ internal bool OnPreFrame() else { var originalFilter = _originalRenderer.GetComponent(); - var filter = _replacementRenderer.GetComponent(); + var filter = Renderer.GetComponent(); filter.sharedMesh = originalFilter.sharedMesh; var shadowBone = ShadowBoneManager.Instance.GetBone(_originalRenderer.transform).proxy; - var rendererTransform = _replacementRenderer.transform; + var rendererTransform = Renderer.transform; if (shadowBone != rendererTransform.parent) { rendererTransform.SetParent(shadowBone, false); @@ -202,7 +203,7 @@ internal bool OnPreFrame() target.enabled = original.enabled && original.gameObject.activeInHierarchy; - _replacementRenderer.sharedMaterials = _originalRenderer.sharedMaterials; + Renderer.sharedMaterials = _originalRenderer.sharedMaterials; target.localBounds = original.localBounds; @@ -239,19 +240,19 @@ internal bool OnPreFrame() internal void FinishPreFrame(bool isSceneViewCamera) { - if (_replacementRenderer != null) + if (Renderer != null) { - var shouldEnable = _replacementRenderer.enabled & !(isSceneViewCamera && _visibilityOffOriginal); + var shouldEnable = Renderer.enabled & !(isSceneViewCamera && _visibilityOffOriginal); Mesh currentSharedMesh = null; - if (_replacementRenderer is SkinnedMeshRenderer smr) + if (Renderer is SkinnedMeshRenderer smr) currentSharedMesh = smr.sharedMesh; - else if (_replacementRenderer is MeshRenderer mr) + else if (Renderer is MeshRenderer mr) currentSharedMesh = mr.GetComponent().sharedMesh; - if (currentSharedMesh != _initialSharedMesh) _replacementRenderer.enabled = false; + if (currentSharedMesh != _initialSharedMesh) Renderer.enabled = false; - _replacementRenderer.enabled = shouldEnable; + Renderer.enabled = shouldEnable; } } @@ -259,7 +260,7 @@ private void CreateReplacementObject() { if (_originalRenderer == null) return; - _replacementRenderer = _cache.GetOrCreate(_originalRenderer, () => + _proxyHandle = _cache.GetHandle(_originalRenderer, () => { var replacementGameObject = new GameObject("Proxy renderer for " + _originalRenderer.gameObject.name); replacementGameObject.hideFlags = HideFlags.DontSave; @@ -292,15 +293,29 @@ private void CreateReplacementObject() return renderer; }); + + _primaryProxy = _proxyHandle.PrimaryProxy; + _setupProxy = _proxyHandle.GetSetupProxy(); + } + + public void FinishSetup() + { + if (_setupComplete) return; + + _setupComplete = true; + _proxyHandle.ReturnSetupProxy(_setupProxy); + _setupProxy = null; } public void Dispose() { - if (_replacementRenderer != null) + if (_setupProxy != null) { - _cache.ReturnProxy(_originalRenderer, _replacementRenderer); - _replacementRenderer = null; + _proxyHandle.ReturnSetupProxy(_setupProxy); } + + _setupProxy = null; + _proxyHandle.Dispose(); } } } \ No newline at end of file diff --git a/Editor/PreviewSystem/Rendering/ProxyPipeline.cs b/Editor/PreviewSystem/Rendering/ProxyPipeline.cs index 6d58805..ddcbca3 100644 --- a/Editor/PreviewSystem/Rendering/ProxyPipeline.cs +++ b/Editor/PreviewSystem/Rendering/ProxyPipeline.cs @@ -209,10 +209,6 @@ private async Task Build(ProxyObjectCache proxyCache, IEnumerable // we render for real). _proxies.Add(r, proxy); - OriginalToProxyRenderer = OriginalToProxyRenderer.Add(r, proxy.Renderer); - OriginalToProxyObject = OriginalToProxyObject.Add(r.gameObject, proxy.Renderer.gameObject); - ProxyToOriginalObject = ProxyToOriginalObject.Add(proxy.Renderer.gameObject, r.gameObject); - var registry = new ObjectRegistry(null); ((IObjectRegistry)registry).RegisterReplacedObject(r, proxy.Renderer); @@ -303,6 +299,15 @@ await Task.WhenAll(_stages.SelectMany(s => s.NodeTasks)) //UnityEngine.Debug.Log($"Total nodes: {total_nodes}, reused: {reused}, refresh failed: {refresh_failed}"); + foreach (var (r, proxy) in _proxies) + { + proxy.FinishSetup(); + + OriginalToProxyRenderer = OriginalToProxyRenderer.Add(r, proxy.Renderer); + OriginalToProxyObject = OriginalToProxyObject.Add(r.gameObject, proxy.Renderer.gameObject); + ProxyToOriginalObject = ProxyToOriginalObject.Add(proxy.Renderer.gameObject, r.gameObject); + } + #if NDMF_DEBUG Debug.Log($"Pipeline {_generation} is ready"); #endif