Skip to content

Conversation

@varinotmUnity
Copy link
Contributor

@varinotmUnity varinotmUnity commented Jan 26, 2026

DO NOT FORGET TO INCLUDE A CHANGELOG ENTRY

Purpose of this PR

follow up of https://github.cds.internal.unity3d.com/unity/unity/pull/91122
I

GetInstanceID is obsolete in 6000_4 and newer.
Also, conversion from entityId to int.

Needed to update tests

Links

N/A

Comments to Reviewers

n/A

@varinotmUnity varinotmUnity self-assigned this Jan 26, 2026
@u-pr
Copy link

u-pr bot commented Jan 26, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

Small, test-only change with a few conditional-compilation branches to validate across Unity versions.
🏅 Score: 80

The intent is clear and the fix is localized, but there’s a likely Unity version define mismatch that could break compilation on some editor versions.
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Build Break

The Unity version scripting define used for the copy mesh ID retrieval differs between tests, which may cause the wrong branch to compile (or not compile) depending on the Unity version; verify the correct define name and keep it consistent.

#if UNITY_6004_0_OR_NEWER
        var copyMeshId = copy.GetComponent<MeshFilter>().sharedMesh.GetObjectId().GetRawData();
#else
        var copyMeshId = (ulong)copy.GetComponent<MeshFilter>().sharedMesh.GetObjectId();
#endif
        Assert.That(copyMeshId, Is.Not.EqualTo(originalMeshId));
Type Semantics

The test now compares mesh IDs as either raw object-id data or a casted unsigned integer; confirm both branches represent the same identity concept across Unity versions and that the casted type matches the actual GetObjectId() return type to avoid false positives/negatives.

#if UNITY_6000_4_OR_NEWER
        var originalMeshId = cube.GetComponent<MeshFilter>().sharedMesh.GetObjectId().GetRawData();
#else
        var originalMeshId = (ulong)cube.GetComponent<MeshFilter>().sharedMesh.GetObjectId();
#endif
Assertion Robustness

Ensure GetRawData() returns a comparable scalar value (not a span/array/struct with reference-based equality) so Is.Not.EqualTo reliably detects different mesh identities.

#if UNITY_6000_4_OR_NEWER
        var copyMeshId = copy.GetComponent<MeshFilter>().sharedMesh.GetObjectId().GetRawData();
#else
        var copyMeshId = (ulong)copy.GetComponent<MeshFilter>().sharedMesh.GetObjectId();
#endif
        Assert.That(copyMeshId, Is.Not.EqualTo(originalMeshId));
    }
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@u-pr
Copy link

u-pr bot commented Jan 26, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix the version define

Fix the preprocessor symbol typo so the same Unity version gate is used consistently
in this test. As written, Unity 6000.4+ will take different branches for
originalMeshId and copyMeshId, which can break compilation or reintroduce the
warning you’re trying to avoid.

Tests/Editor/Editor/MeshSyncTests.cs [133-137]

-#if UNITY_6004_0_OR_NEWER
+#if UNITY_6000_4_OR_NEWER
         var copyMeshId = copy.GetComponent<MeshFilter>().sharedMesh.GetObjectId().GetRawData();
 #else
         var copyMeshId = (ulong)copy.GetComponent<MeshFilter>().sharedMesh.GetObjectId();
 #endif
Suggestion importance[1-10]: 8

__

Why: The #if symbol UNITY_6004_0_OR_NEWER is inconsistent with the earlier UNITY_6000_4_OR_NEWER checks, which can cause the test to compile different branches (or fail to compile) for originalMeshId vs copyMeshId.

Medium
  • More suggestions

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@thomas-tu thomas-tu changed the title update tests to fix instanceIDwarnings Update tests to fix InstanceID warnings Jan 26, 2026
@codecov-github-com
Copy link

codecov-github-com bot commented Jan 26, 2026

Codecov Report

All modified and coverable lines are covered by tests ✅

@@           Coverage Diff           @@
##           master     #646   +/-   ##
=======================================
  Coverage   36.05%   36.05%           
=======================================
  Files         277      277           
  Lines       34900    34900           
=======================================
  Hits        12584    12584           
  Misses      22316    22316           
Flag Coverage Δ
mac_trunk 34.59% <ø> (ø)
win_trunk 34.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

#else
return (ulong)id;
#endif
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of casting an int into a ulong - which can cause loss of precision somwhere, you could define an ID type based on the #ifdef

#if UNITY_6000_4_OR_NEWER
using UObjectID = UnityEngine.EntityId;
#else
using UObjectID = System.Int32;
#endif

@thomas-tu thomas-tu merged commit f96581d into master Jan 30, 2026
10 checks passed
@thomas-tu thomas-tu deleted the bugfix/test-entity-id-fix branch January 30, 2026 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants