[UUM-137643] Fix compilation errors and warnings #627
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| using System.IO; | ||
| using System.Collections.Generic; | ||
| using System.Reflection; | ||
| using System.Text; | ||
| #if UNITY_EDITOR | ||
| using UnityEditor; | ||
| #endif | ||
|
|
@@ -546,7 +547,11 @@ public override void Setup(Component c) | |
| m_target = null; | ||
| return; | ||
| } | ||
| #if UNITY_6000_4_OR_NEWER | ||
| abcObject = parent.abcObject.NewXform(target.name + " (" + EntityId.ToULong(target.GetEntityId()).ToString("X16") + ")", timeSamplingIndex); | ||
| #else | ||
| abcObject = parent.abcObject.NewXform(target.name + " (" + target.GetInstanceID().ToString("X8") + ")", timeSamplingIndex); | ||
| #endif | ||
| m_target = target; | ||
| } | ||
|
|
||
|
|
@@ -834,7 +839,11 @@ public void Dispose() | |
|
|
||
| class CaptureNode | ||
| { | ||
| #if UNITY_6000_4_OR_NEWER | ||
| public ulong entityId; | ||
| #else | ||
| public int instanceID; | ||
| #endif | ||
| public Type componentType; | ||
| public CaptureNode parent; | ||
| public Transform transform; | ||
|
|
@@ -872,9 +881,15 @@ class CapturerRecord | |
|
|
||
| aeContext m_ctx; | ||
| ComponentCapturer m_root; | ||
| #if UNITY_6000_4_OR_NEWER | ||
| Dictionary<ulong, CaptureNode> m_nodes; | ||
| List<CaptureNode> m_newNodes; | ||
| List<ulong> m_entityIdsToRemove; | ||
| #else | ||
| Dictionary<int, CaptureNode> m_nodes; | ||
| List<CaptureNode> m_newNodes; | ||
| List<int> m_iidToRemove; | ||
| #endif | ||
| int m_lastTimeSamplingIndex; | ||
| int m_startFrameOfLastTimeSampling; | ||
|
|
||
|
|
@@ -947,16 +962,96 @@ public static void ForceDisableBatching() | |
|
|
||
| #endif | ||
|
|
||
| #if UNITY_6000_4_OR_NEWER | ||
|
Collaborator
Author
There was a problem hiding this comment. Explaination about why we need this:
Contributor
There was a problem hiding this comment. A concern that jumped out to me is performance. Also out of curiosity why is determinism important here? What would it mean not having determinism?
Collaborator
Author
There was a problem hiding this comment. Good questions :D Put on your seat belt 😄 Some context GetTargets (where this func is used) is the scene discovery mechanism. Its results feed ConstructTree, which inserts nodes into m_nodes aka builds the in-memory capture tree and ultimately determines the order in which Alembic nodes (NewXform, NewPolyMesh, etc.) are created inside the archive. GetTargets is called at every capture frame during recording. Why we want determinism The tree topology is naturaly done and deterministic for parents/children because of the recursiveness used (aka we always resolves parents before children). But what is not naturally stable is the sibling ordering, aka the order in which NewXform/NewPolyMesh is called on the children of a given parent, which determines the order those children appear in the Alembic file. If no sorting is done, sibling order will not be stable and produce .abc files that bit-wise/ascii-wise looks different when they technically are not (noise during versioning, unnessary cache invalidation...). BTW the sorting by InstanceID was actually not great either, because from a Unity session to another, the instanceID could change and so the ordering of the sibblings. The new sorting is based on the GO scene path and root-to-leaf sibling indices (that order is serialized into the scene file and restored identically every time the scene loads), this is now a structural and session-independent sorting. Perf concern At every captured frame, we unconditionally calls UpdateCaptureNodes, which calls GetTargets once per enabled capturer type (up to 4: Transform, Camera, MeshRenderer, SkinnedMeshRenderer) = SortComponentsByStableSceneHierarchy runs up to 4× per frame for the entire duration of recording 😅 So, ya, not great, the new sorting is probably worse than native InstanceID sorting.
Collaborator
Author
There was a problem hiding this comment. Well, I tried for few hours but the improvements I could get (without breaking everything) are almost anecdoctical and there is still this idea of minimalizing impact / halo in the changes we do in this package. To mitigate, this is only run when we record. So I would stop there. How about that @LeoUnity ? |
||
| /// <summary> | ||
| /// Sorts <paramref name="components"/> in place into a stable order derived from each object’s scene and | ||
| /// transform hierarchy (see <see cref="AppendStableHierarchySortKey(StringBuilder, Component, List{int})"/>). | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// On Unity 6.4 and newer, sort mode is deprecated for <c>FindObjectsByType</c>, so the returned array has no | ||
| /// defined order. Sorting by <c>InstanceID</c> / <c>EntityId</c> is also discouraged. This method reorders the | ||
| /// array so exporter iteration (for example from <c>GetTargets</c>) is deterministic and aligned with scene and | ||
| /// hierarchy rather than identity allocation. | ||
| /// </para> | ||
| /// <para> | ||
| /// Implementation: allocate a parallel <c>string[]</c> of the same length, fill each entry by clearing a shared | ||
| /// <see cref="StringBuilder"/> and a shared <c>List<int></c> scratchpad, calling | ||
| /// <see cref="AppendStableHierarchySortKey(StringBuilder, Component, List{int})"/>, and assigning | ||
| /// <c>keys[i] = sb.ToString()</c>. Then <c>Array.Sort(keys, components, StringComparer.Ordinal)</c> | ||
| /// reorders <paramref name="components"/> to match ascending key order. Arrays of length 0 or 1, or a null | ||
| /// reference, are left unchanged without allocating. | ||
| /// </para> | ||
| /// </remarks> | ||
| internal static void SortComponentsByStableSceneHierarchy(Component[] components) | ||
| { | ||
| if (components == null || components.Length <= 1) | ||
| return; | ||
|
|
||
| var keys = new string[components.Length]; | ||
| var sb = new StringBuilder(128); | ||
| var scratchpad = new List<int>(8); | ||
| for (int i = 0; i < components.Length; i++) | ||
| { | ||
| sb.Clear(); | ||
| scratchpad.Clear(); | ||
| AppendStableHierarchySortKey(sb, components[i], scratchpad); | ||
| keys[i] = sb.ToString(); | ||
| } | ||
|
|
||
| Array.Sort(keys, components, StringComparer.Ordinal); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Appends a deterministic sort key for <paramref name="c"/> to <paramref name="sb"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// Unity 6.4+ no longer guarantees an ordering for <c>FindObjectsByType</c>, and engine guidance is to avoid | ||
| /// sorting by <c>InstanceID</c> / <c>EntityId</c>. This key instead fixes a stable order from scene identity | ||
| /// and transform hierarchy so export iteration is reproducible. | ||
| /// </para> | ||
| /// <para> | ||
| /// The key is built as: <c>scene.path</c>, a U+0001 separator, then the path from scene root to the component’s transform as dot-separated | ||
| /// <see cref="Transform.GetSiblingIndex"/> values, each formatted as eight digits (<c>D8</c>) so lexical | ||
| /// string order matches numeric sibling order. Walking from leaf to root and emitting indices root-first yields | ||
| /// depth-first hierarchy order when keys are compared with <see cref="StringComparer.Ordinal"/>. | ||
| /// </para> | ||
| /// </remarks> | ||
| internal static void AppendStableHierarchySortKey(StringBuilder sb, Component c, List<int> scratchpad) | ||
| { | ||
| var scene = c.gameObject.scene; | ||
| sb.Append(scene.path); | ||
| sb.Append('\x1'); | ||
| var t = c.transform; | ||
| while (t != null) | ||
| { | ||
| scratchpad.Add(t.GetSiblingIndex()); | ||
| t = t.parent; | ||
| } | ||
| for (int i = scratchpad.Count - 1; i >= 0; i--) | ||
| { | ||
| if (i < scratchpad.Count - 1) | ||
| sb.Append('.'); | ||
| sb.Append(scratchpad[i].ToString("D8")); | ||
| } | ||
| } | ||
| #endif | ||
|
|
||
| Component[] GetTargets(Type type) | ||
| { | ||
| if (m_settings.Scope == ExportScope.TargetBranch && TargetBranch != null) | ||
| return TargetBranch.GetComponentsInChildren(type); | ||
|
|
||
| #if UNITY_6000_4_OR_NEWER | ||
| var objects = GameObject.FindObjectsByType(type); | ||
| var components = Array.ConvertAll<UnityEngine.Object, Component>(objects, e => (Component)e); | ||
| SortComponentsByStableSceneHierarchy(components); | ||
| return components; | ||
| #elif UNITY_2023_1_OR_NEWER | ||
| return Array.ConvertAll<UnityEngine.Object, Component>(GameObject.FindObjectsByType(type, FindObjectsSortMode.InstanceID), e => (Component)e); | ||
| #else | ||
| return Array.ConvertAll<UnityEngine.Object, Component>(GameObject.FindObjectsOfType(type), e => (Component)e); | ||
| #endif | ||
| } | ||
|
|
||
|
|
@@ -974,15 +1069,29 @@ CaptureNode ConstructTree(Transform node) | |
| { | ||
| if (node == null) { return null; } | ||
|
|
||
| #if UNITY_6000_4_OR_NEWER | ||
| ulong entityId = EntityId.ToULong(node.gameObject.GetEntityId()); | ||
| #else | ||
| int iid = node.gameObject.GetInstanceID(); | ||
| #endif | ||
| CaptureNode cn; | ||
| #if UNITY_6000_4_OR_NEWER | ||
| if (m_nodes.TryGetValue(entityId, out cn)) { return cn; } | ||
|
|
||
| cn = new CaptureNode(); | ||
| cn.entityId = entityId; | ||
| cn.transform = node; | ||
| cn.parent = ConstructTree(node.parent); | ||
| m_nodes.Add(entityId, cn); | ||
| #else | ||
| if (m_nodes.TryGetValue(iid, out cn)) { return cn; } | ||
|
|
||
| cn = new CaptureNode(); | ||
| cn.instanceID = iid; | ||
| cn.transform = node; | ||
| cn.parent = ConstructTree(node.parent); | ||
| m_nodes.Add(iid, cn); | ||
| #endif | ||
| m_newNodes.Add(cn); | ||
| return cn; | ||
| } | ||
|
|
@@ -1028,9 +1137,15 @@ void SetupComponentCapturer(CaptureNode node) | |
| if (parent != null && parent.transformCapturer == null) | ||
| { | ||
| SetupComponentCapturer(parent); | ||
| #if UNITY_6000_4_OR_NEWER | ||
| if (!m_nodes.ContainsKey(parent.entityId) || !m_newNodes.Contains(parent)) | ||
| { | ||
| m_nodes.Add(parent.entityId, parent); | ||
| #else | ||
| if (!m_nodes.ContainsKey(parent.instanceID) || !m_newNodes.Contains(parent)) | ||
| { | ||
| m_nodes.Add(parent.instanceID, parent); | ||
| #endif | ||
| m_newNodes.Add(parent); | ||
| } | ||
| } | ||
|
|
@@ -1143,9 +1258,15 @@ public bool BeginRecording() | |
| } | ||
|
|
||
| m_root = new RootCapturer(this, m_ctx.topObject); | ||
| #if UNITY_6000_4_OR_NEWER | ||
| m_nodes = new Dictionary<ulong, CaptureNode>(); | ||
| m_newNodes = new List<CaptureNode>(); | ||
| m_entityIdsToRemove = new List<ulong>(); | ||
| #else | ||
| m_nodes = new Dictionary<int, CaptureNode>(); | ||
| m_newNodes = new List<CaptureNode>(); | ||
| m_iidToRemove = new List<int>(); | ||
| #endif | ||
| m_lastTimeSamplingIndex = 1; | ||
| m_startFrameOfLastTimeSampling = 0; | ||
|
|
||
|
|
@@ -1170,7 +1291,11 @@ public void EndRecording() | |
| { | ||
| if (!m_recording) { return; } | ||
|
|
||
| #if UNITY_6000_4_OR_NEWER | ||
| m_entityIdsToRemove = null; | ||
| #else | ||
| m_iidToRemove = null; | ||
| #endif | ||
| m_newNodes = null; | ||
| m_nodes = null; | ||
| m_root = null; | ||
|
|
@@ -1213,14 +1338,24 @@ public void ProcessRecording() | |
| var node = kvp.Value; | ||
| node.Capture(); | ||
| if (node.transform == null) | ||
| #if UNITY_6000_4_OR_NEWER | ||
| m_entityIdsToRemove.Add(node.entityId); | ||
| #else | ||
| m_iidToRemove.Add(node.instanceID); | ||
| #endif | ||
| } | ||
| m_ctx.MarkFrameEnd(); | ||
|
|
||
| // remove deleted GameObjects | ||
| #if UNITY_6000_4_OR_NEWER | ||
| foreach (ulong entityId in m_entityIdsToRemove) | ||
| m_nodes.Remove(entityId); | ||
| m_entityIdsToRemove.Clear(); | ||
| #else | ||
| foreach (int iid in m_iidToRemove) | ||
| m_nodes.Remove(iid); | ||
| m_iidToRemove.Clear(); | ||
| #endif | ||
|
|
||
| // advance time | ||
| ++m_frameCount; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we also remove this and upgrade the minimum unity version to 6000.0?
Same with romotion_test_editors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the minimum version of the package, so we have to keep the job for promotion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe its allow to bump the minimum version of the package if said version is not supported anymore without being considered a new major release. But if that would increase the scope of this PR, we can do it separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it's indeed out of scope, so let's log a task to update this separately.