-
Notifications
You must be signed in to change notification settings - Fork 329
FIX: Address warnings that happened with Unity 6.4 #2233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2233 +/- ##
===========================================
- Coverage 68.17% 68.15% -0.02%
===========================================
Files 367 367
Lines 53668 53685 +17
===========================================
+ Hits 36587 36591 +4
- Misses 17081 17094 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
d57e958
to
24bda46
Compare
// See https://jira.unity3d.com/browse/XR-7591 | ||
#pragma warning disable CS0618 | ||
UnityEngine.XR.XRDevice.DisableAutoXRCameraTracking(cameraComponent, true); | ||
#pragma warning restore CS0618 |
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.
Also a Slack thread: https://unity.slack.com/archives/C06TQE84B/p1757416691328499
#if UNITY_6000_4_OR_NEWER | ||
public static bool OpenAsset(EntityId entityId, int line) | ||
{ | ||
if (!InputActionImporter.IsInputActionAssetPath(AssetDatabase.GetAssetPath(entityId))) |
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.
Now that I read this code myself, I could have left IsInputActionAssetPath in OpenAsset, just the GetAssetPath thing is different. IDK, either way goes fine by my books
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.
Minor: But I would agree with you, that if it makes the "fork condition" slimmer it is a good idea.
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.
But its also fine as-is
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.
Pull Request Overview
This PR addresses compilation warnings that occur when using the Input System package with Unity 6.4 by updating deprecated APIs and maintaining backward compatibility with earlier Unity versions.
- Updates the deprecated
instanceID
API to use the newentityID
API introduced in Unity 6.4 - Adds pragma warnings to suppress deprecation warnings for XR device APIs that don't have replacements yet
- Implements conditional compilation to maintain compatibility across Unity versions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
MiscHelpers.cs | Adds UnityEditor using statement for editor functionality |
TrackedPoseDriver.cs | Adds pragma warnings around deprecated XR API calls with explanatory comments |
InputSystem.cs | Implements HasNativeObject helper method with conditional compilation for instanceID/entityID migration |
InputActionsEditorWindow.cs | Updates OnOpenAsset method to handle both instanceID and entityID based on Unity version |
InputActionEditorWindow.cs | Updates OnOpenAsset method to handle both instanceID and entityID based on Unity version |
CHANGELOG.md | Documents the fix for Unity 6.4 compilation warnings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Packages/com.unity.inputsystem/InputSystem/Utilities/MiscHelpers.cs
Outdated
Show resolved
Hide resolved
…rs.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Good catches on this PR by Copilot |
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 think this looks good. Just some thoughts and minor comments that if they work could reduce the diffs and forks-conditionals.
#if UNITY_6000_4_OR_NEWER | ||
public static bool OpenAsset(EntityId entityId, int line) | ||
{ | ||
if (!InputActionImporter.IsInputActionAssetPath(AssetDatabase.GetAssetPath(entityId))) |
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.
Minor: But I would agree with you, that if it makes the "fork condition" slimmer it is a good idea.
#if UNITY_6000_4_OR_NEWER | ||
public static bool OpenAsset(EntityId entityId, int line) | ||
{ | ||
if (!InputActionImporter.IsInputActionAssetPath(AssetDatabase.GetAssetPath(entityId))) |
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.
But its also fine as-is
} | ||
|
||
// We have this function to hide away instanceId -> entityId migration that happened in Unity 6.4 | ||
public static bool HasNativeObject(Object obj) |
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.
Minor: Hmmm.... This diff was hard to review but just a thought.
What if this code was changed so that HasNativeObject(Object)
would call another function:
Object GetObjectFromEntityID(EntityId id) { <same #if dance to map to EditorUtility> }
And then using a conditional alias where needed for EntityId, e.g.
#if !UNITY_6000_4_OR_NEWER
using EntityId = int;
#endif
Would that eliminate all other branches?
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.
Maybe it just makes things weird if entity id and instance id are fundamentally different, if not maybe its something....
Description
This fixes up the current set of warnings that we get when using the package with Unity 6.4.
Related to https://jira.unity3d.com/browse/ISX-2349
Testing status & QA
Local testing
Overall Product Risks
Checklist
Before review:
Changed
,Fixed
,Added
sections.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.During merge:
NEW: ___
.FIX: ___
.DOCS: ___
.CHANGE: ___
.RELEASE: 1.1.0-preview.3
.After merge: