[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,109 @@ 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 ?
Contributor
There was a problem hiding this comment. Seat Belts on 🎽 😁 That makes total sense, thanks for bringing me up to speed on this! Let's just confirm expectations users might have around this package, and more specifically when this code is running to make sure we can pick an appropriate path forward. If the expectation is that this is an exporter to be done during edit time, or maybe at runtime but as an export task where the user don't expect to still interact with the application while it's happening, then I think it's fine to take this added hit for determinism. If there are expectations that this export needs to happen "instantly" or in async/background while the application maintains interactive speeds, then we might want to do some perf exploration to understand the impact. I'm assuming expectations are that exporting an alembic file is an expensive operation, I'm also assuming that exporting an alembic file is a synchronous main-thread operation that scales linearly with scene complexity, so there is no expectations that the editor or a player will keep interactive frame-rates while the export is happening. If this all holds, then I think its find to take a perf hit in favor of better determinism, this is the correct trade-off. But please let me know if any of my assumptions doesn't hold!
Collaborator
Author
There was a problem hiding this comment. Great questions :D Alembic exporters (both the Alembic Exporter component and the Alembic recorder) work similarly as Recorder: you set-up the exporter in Edit mode but the export itself is only happening in Play mode. My understanding is most of the time, the Alembic Importer / Exporter is used for round-tripping during production, but using it in a build is a valid use case. In both case, there is no expectation that the export will be realtime and it is indeed expected that the deepest/complexe the hierarchy is, the slower it will be. As a matter of fact, the cost added by the sort is small as compared to the FindObjectsByType or the BakeMesh already done. But that's another topic and I agree with you that true determinism is worth the trade off. That said, I pushed a tiny improv to remove come GC allocs. I'm not sure it is worth to bloat the code but, ya 😅 |
||
| /// <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('.'); | ||
| AppendD8(sb, scratchpad[i]); | ||
| } | ||
| } | ||
|
|
||
| // Appends value zero-padded to 8 digits directly into sb with no string allocation. | ||
| static void AppendD8(StringBuilder sb, int value) | ||
| { | ||
| sb.Append((char)('0' + value / 10000000 % 10)); | ||
| sb.Append((char)('0' + value / 1000000 % 10)); | ||
| sb.Append((char)('0' + value / 100000 % 10)); | ||
| sb.Append((char)('0' + value / 10000 % 10)); | ||
| sb.Append((char)('0' + value / 1000 % 10)); | ||
| sb.Append((char)('0' + value / 100 % 10)); | ||
| sb.Append((char)('0' + value / 10 % 10)); | ||
| sb.Append((char)('0' + value % 10)); | ||
| } | ||
| #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 +1082,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 +1150,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 +1271,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 +1304,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 +1351,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.