Skip to content
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

Jump to bottom

CPU Performance Improvements #278

Merged
merged 3 commits into from Jun 6, 2023
Merged

CPU Performance Improvements #278

merged 3 commits into from Jun 6, 2023

Conversation

smitdylan2001
Copy link
Contributor

@smitdylan2001 smitdylan2001 commented Jun 6, 2023 •
edited

No changes were made a change to functionality, only performance improvement.

  • Use simpler return statements in WebXRManager & WebXRPackage (very small improvement)
  • Use .CompareTag instead of string comparison in ControllerInteraction (no garbage allocation)
  • Use TryGetComponent in Controller Interaction (no garbage allocation when component is null)
  • Use SetLocalPositionAndRotation instead of 2 separate calls. This changes the amount of time a hierarchy needs to be calculated from 2 to 1, improving CPU substantially for big hierarchies.

transform.SetLocalPositionAndRotation is the biggest changeAdded in 2021.3.11, but since there is no compilation check except #if UNITY_2021_3_OR_NEWER, people using version .0 to .10 would get compilation errors.
#if UNITY_2021_3_OR_NEWER can be used since updates within the same LTS version rarely break the project, but for safety not included in the PR yet.

@smitdylan2001
Performance Improvements
eb70442 
- Use simpler return statement in WebXRManager & WebXRPackage (very small improvement)
- Use .CompareTag instead of stringComparison in ControllerInteraction (no garbage allocation)
- Use TryGetComponent in Controller Interaction (no garbage allocation when component is null)
- Use SetLocalPositionAndRotation instead of 2 seperate calls. This changes the amount of time a hierarchy needs to be calculated from 2 to 1, improving CPU substantially for big hierarchies.
Added in 2021.3.11, but since there is no compilation check except #if UNITY_2021_3_OR_NEWER, people using version .0 to .10 would get compilation errors. This can be changed, as upgrades within LTS are relatively safe and they can be told to upgrade.
@De-Panther
Copy link
Owner

Awesome! Thanks!

De-Panther
De-Panther reviewed Jun 6, 2023
View reviewed changes
Copy link
Owner

@De-Panther De-Panther left a comment

Choose a reason for hiding this comment

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

Looks great, just some indentation changes needed

Packages/webxr-interactions/Runtime/Scripts/ControllerInteraction.cs Outdated Show resolved Hide resolved
Packages/webxr-interactions/Runtime/Scripts/ControllerInteraction.cs Outdated Show resolved Hide resolved
Packages/webxr-interactions/Runtime/Scripts/ControllerInteraction.cs Outdated Show resolved Hide resolved
Packages/webxr-interactions/Runtime/Scripts/ControllerInteraction.cs Outdated Show resolved Hide resolved
Packages/webxr-interactions/Runtime/Scripts/ControllerInteraction.cs Outdated Show resolved Hide resolved
Packages/webxr/Runtime/Scripts/WebXRController.cs Outdated Show resolved Hide resolved
Packages/webxr/Runtime/Scripts/WebXRController.cs Outdated Show resolved Hide resolved
Packages/webxr/Runtime/Scripts/WebXRController.cs Outdated Show resolved Hide resolved
Packages/webxr/Runtime/Scripts/WebXRController.cs Outdated Show resolved Hide resolved
Packages/webxr/Runtime/Scripts/WebXRController.cs Outdated Show resolved Hide resolved
De-Panther
De-Panther reviewed Jun 6, 2023
View reviewed changes
Copy link
Owner

@De-Panther De-Panther left a comment

Choose a reason for hiding this comment

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

More indentation changes

Packages/webxr-interactions/Runtime/Scripts/ControllerInteraction.cs Outdated Show resolved Hide resolved
Packages/webxr/Runtime/Scripts/WebXRController.cs Outdated Show resolved Hide resolved
@De-Panther
Copy link
Owner

🎉

@De-Panther De-Panther merged commit bc3d2ab into De-Panther:master Jun 6, 2023
@smitdylan2001
Copy link
Contributor Author

Thanks for making the changes yourself!
Doing some awesome work with the exporter

@smitdylan2001
Copy link
Contributor Author

@De-Panther

It would be possible to use a custom # define at the top of each script with all the version checks, so 2021LTS users also can use the new feature without changing script. This might start to look even more cluttered however. If you think it's a good idea I could add it to a new PR.

Info:
https://forum.unity.com/threads/minor-version-or_newer-compilation.1445305

@De-Panther
Copy link
Owner

The issue is that it's both on 2021.3.11 and up in the 2021.3.x cycle, but then skips 2022.1.x and 2022.2.x cycles.
Maybe setting it in the asmdef file would look better?
https://docs.unity3d.com/Manual/ScriptCompilationAssemblyDefinitionFiles.html
"Defining symbols based on Unity and project package versions"
"Resource: choose Unity or the package or module that must be installed in order for this symbol to be defined"

@smitdylan2001
Copy link
Contributor Author

Oh that's a big oversight of me indeed!
I am looking into the asmdef defines, but am unsure how to make it be so selectable.
For now maybe keeping it in 2022.3 is the best option unless a good solution pops up

@De-Panther
Copy link
Owner

For now 2022.3 is ok.
That's why I approved and merged :)
Thanks again

@De-Panther De-Panther mentioned this pull request Jun 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@De-Panther De-Panther De-Panther approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@smitdylan2001 @De-Panther