[UUM-137643] Fix compilation errors and warnings by mfe · Pull Request #627 · Unity-Technologies/com.unity.formats.alembic

mfe · 2026-03-24T15:12:30Z

Purpose of this PR

Ticket/Jira #: UUM-137643

This PR address 3 issues:

1 - Compilation errors in the console

error CS0619: 'Object.GetInstanceID()' is obsolete: 'GetInstanceID is deprecated. Use GetEntityId instead.

2 - Warnings (that will become errors in a foreseeable futur):

warning CS0618: 'FindObjectsSortMode' is obsolete: 'FindObjectsSortMode has been deprecated. Use the FindObjectsByType overloads that do not take a FindObjectsSortMode parameter.'
⚠️ In previous versions of the code, a sorting by FindObjectsSortMode.instanceID was used to have the determinism needed for UpdateCaptureNodes. But that was an inconsistant way of sorting. Furthermore, with entityID, the small amount of creation-order semantics that were accidentally present in instanceID is removed. That's why this PR instroduces another way of sorting with determinism (cf SortComponentsByStableSceneHierarchy).

3- replace renderPipelineAsset by defaultRenderPipeline

defautRenderPipeline was introduced in 2019 stream so that was due.

A general mindset for this PR is the less we touch, the better.
The reason is that this code is very legacy and in basic support. We want to mitigate the risk of breaking the past versions and increase our support load.
That's why every changes needed were encapsulated in #if UNITY_6000_4_OR_NEWER...#endif even when the changes would be considered a better practice for precedent versions too (like not sorting with instanceIds).

Testing

Functional Testing status:

Performance Testing status:

Overall Product Risks

Complexity:

Halo Effect:

Documentation & UX Writing

User facing text to review		Details
User interface
Public API docs
Changelog

Documentation halo effect		Jira link
User manual
Other docs

Additional information

Note to reviewers:

Reminder:

Add entry in CHANGELOG.md
check defaultRenderPipeline availability - Was introduced as early as 2019.4 so before our minimal supported version (21.3)
Wait for Recorder 5.1.6 to be released (see UUM-137641 - Cf Recorder tests
Sanity check QA - test import + export both for local and streamed assets with depth

LeoUnity · 2026-04-07T09:07:19Z

.yamato/global.metafile

@@ -1,24 +1,24 @@
 test_editors:
  - version: 2021.3


shouldn't we also remove this and upgrade the minimum unity version to 6000.0?

Same with romotion_test_editors

It's the minimum version of the package, so we have to keep the job for promotion.

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

I would say it's indeed out of scope, so let's log a task to update this separately.

LeoUnity

Changes looks good to me, we do have a test failure on win trunk unfortunately.

mfe · 2026-04-07T15:02:26Z

com.unity.formats.alembic/Runtime/Scripts/Analytics/AlembicExporterAnalytics.cs

            }
-#if UNITY_2023_1_OR_NEWER
+#if UNITY_6000_4_OR_NEWER
+            return Object.FindObjectsByType<T>();


The FindObjectsSortMode.InstanceID used in UNITY_2023_1_OR_NEWER code path is not needed as the result of GetTargets is only used with Any(), so I just removed the sorting mode here.

I was tempted to remove it in UNITY_2023_1_OR_NEWER code path too but on a very legacy package in basic support, the less we touch, the better.
Feel free to push back.

com.unity.formats.alembic/Runtime/Scripts/Exporter/AlembicExporter_impl.cs

mfe · 2026-04-10T13:00:05Z

com.unity.formats.alembic/Runtime/Scripts/Importer/AlembicStream.cs

        {
            m_time = 0.0f;
+#if UNITY_6000_4_OR_NEWER
+            m_context = new SafeContext(aiContext.Create(AllocateNativeContextUid()));


The native function called at the end of the line is aiContextCreate(int uid). So if we keep using a ulong here, we ultimately have to cast it, which can lead to some narrowing and rare but potential collisions.
But in the code where the id is used, there is no need for a correlation between the id and the abcTreeRoot object, we just need an id to destroy on dispose (cf aiContextDestroy(aiContext* ctx)).

Thus the introduction of an alternative surrogate id.

A question for reviewers: what is the bigger risk ?
1- Changing this code
2- Narrowing and collision
😅

I think this approach you took is great!

mfe · 2026-04-10T13:20:26Z

com.unity.formats.alembic/Runtime/Scripts/Exporter/AlembicExporter_impl.cs


 #endif

+#if UNITY_6000_4_OR_NEWER


Explaination about why we need this:
⚠️ In previous versions of the code, a sorting by FindObjectsSortMode.instanceID was used to have the determinism needed for UpdateCaptureNodes. But that was an inconsistant way of sorting. Furthermore, with entityID, the small amount of creation-order semantics that was accidentally present in instanceID is removed. That's why this PR instroduces another way of sorting with determinism,

A concern that jumped out to me is performance.
When and how often is this sorting taking place? We are doing a lot more computations including string operations to get this determinism.

Also out of curiosity why is determinism important here? What would it mean not having determinism?

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.
That said there is room for improv, let me try.

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.
Feel free to push back if you feel I should persevere in trying to improve the perf.

How about that @LeoUnity ?

LeoUnity · 2026-04-10T15:28:49Z

com.unity.formats.alembic/Runtime/Scripts/Importer/AlembicStream.cs

+        // Theoretical int overflow after ~2 billion AbcLoad calls is impossible in practice.
+        static int s_nextNativeContextUid;
+
+        static int AllocateNativeContextUid() => Interlocked.Increment(ref s_nextNativeContextUid);


Should/could this be uint? we are leaving half of its range on the table

I'm using the type of the native function that this id is for (aka aiContextCreate(int uid))

Upgrade editor versions in CI

f5e2185

mfe changed the title ~~Upgrade editor versions in CI~~ [UUM-137643] Fix compilation errors and warnings Mar 24, 2026

mfe added 2 commits March 24, 2026 16:43

Use defaultRenderPipeline instead of renderPipelineAsset

b0bd17c

Remove usage of obsolete FindObjectsSortMode for 6.4+

42224b6

mfe marked this pull request as ready for review March 24, 2026 16:06

mfe requested review from a team as code owners March 24, 2026 16:06

mfe added 2 commits April 3, 2026 15:43

Upgrade and changelog

33c5f94

Use GetEntityId for 6.4+

52abb9f

mfe force-pushed the UUM-137643_fix_coreclr_errors branch from b8c4202 to 52abb9f Compare April 3, 2026 14:02

LeoUnity reviewed Apr 7, 2026

View reviewed changes

LeoUnity approved these changes Apr 7, 2026

View reviewed changes

UseSort by EntityID to have the same deterministic behaviour as before

12f0896

mfe force-pushed the UUM-137643_fix_coreclr_errors branch from 626a370 to 12f0896 Compare April 7, 2026 14:54

mfe commented Apr 7, 2026

View reviewed changes

mfe added 2 commits April 9, 2026 16:12

Remove discouraged sorted by entityId

7676fc0

Use ulong instead of casting

827f368

mfe commented Apr 9, 2026

View reviewed changes

com.unity.formats.alembic/Runtime/Scripts/Exporter/AlembicExporter_impl.cs Outdated Show resolved Hide resolved

mfe added 3 commits April 10, 2026 11:40

Add tests for StableHierarchy sort

605ae76

Improve docstr on the new funcs

2f00533

Replace entityId usage by a plain id

6635346

mfe commented Apr 10, 2026

View reviewed changes

mfe requested review from LeoUnity and thomas-tu April 10, 2026 13:52

Some polish

2c656ba

LeoUnity reviewed Apr 10, 2026

View reviewed changes

Conversation

mfe commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

1 - Compilation errors in the console

2 - Warnings (that will become errors in a foreseeable futur):

3- replace renderPipelineAsset by defaultRenderPipeline

Testing

Overall Product Risks

Documentation & UX Writing

Additional information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeoUnity left a comment

Choose a reason for hiding this comment

Uh oh!

mfe Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfe Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mfe commented Mar 24, 2026 •

edited

Loading

mfe Apr 7, 2026 •

edited

Loading

mfe Apr 14, 2026 •

edited

Loading