Skip to content

Commit

Permalink
fix: flickering selection outlines
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bdunderscore
bdunderscore committed Oct 15, 2024
1 parent 6bafaac commit bf41d33
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 84 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
148 changes: 89 additions & 59 deletions Editor/PreviewSystem/Rendering/ProxyObjectCache.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Renderer> _createFunc;
private readonly RendererState _state;

public ProxyHandleImpl(ProxyObjectCache cache, Renderer key, Func<Renderer> 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<Renderer, RendererState> _renderers = new();
private bool _cleanupPending = false;
private bool _isRegistered = false;

private bool IsRegistered
Expand All @@ -37,10 +97,12 @@ private bool IsRegistered
if (value)
{
EditorApplication.playModeStateChanged += OnPlayModeStateChanged;
EditorApplication.update += Cleanup;
}
else
{
EditorApplication.playModeStateChanged -= OnPlayModeStateChanged;
EditorApplication.update -= Cleanup;
}

_isRegistered = value;
Expand All @@ -53,69 +115,26 @@ private void OnPlayModeStateChanged(PlayModeStateChange obj)
Dispose();
}

public Renderer GetOrCreate(Renderer original, Func<Renderer> create)
public IProxyHandle GetHandle(Renderer original, Func<Renderer> 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)
Expand All @@ -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);
}
}
Expand All @@ -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;
Expand Down
57 changes: 36 additions & 21 deletions Editor/PreviewSystem/Rendering/ProxyObjectController.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

using System;
using System.Linq;
using System.Threading.Tasks;
using UnityEditor;
using UnityEngine;
using UnityEngine.Profiling;
Expand All @@ -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;

Expand Down Expand Up @@ -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...");
}
Expand All @@ -145,7 +146,7 @@ internal bool OnPreFrame()

try
{
var target = _replacementRenderer;
var target = Renderer;
var original = _originalRenderer;

if (VisibilityMonitor.Sequence != _lastVisibilityCheck)
Expand Down Expand Up @@ -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;

Expand All @@ -186,12 +187,12 @@ internal bool OnPreFrame()
else
{
var originalFilter = _originalRenderer.GetComponent<MeshFilter>();
var filter = _replacementRenderer.GetComponent<MeshFilter>();
var filter = Renderer.GetComponent<MeshFilter>();
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);
Expand All @@ -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;
Expand Down Expand Up @@ -239,27 +240,27 @@ 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<MeshFilter>().sharedMesh;

if (currentSharedMesh != _initialSharedMesh) _replacementRenderer.enabled = false;
if (currentSharedMesh != _initialSharedMesh) Renderer.enabled = false;

_replacementRenderer.enabled = shouldEnable;
Renderer.enabled = shouldEnable;
}
}

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;
Expand Down Expand Up @@ -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();
}
}
}
13 changes: 9 additions & 4 deletions Editor/PreviewSystem/Rendering/ProxyPipeline.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,6 @@ private async Task Build(ProxyObjectCache proxyCache, IEnumerable<IRenderFilter>
// 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);
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit bf41d33

Please sign in to comment.