Implement camera cache utility class #852

Zod- · 2017-08-12T17:16:00Z

Fixes #848

This adds a new Utility class that allows the use a cached reference to the main camera via CameraCache.main. Additionally, a method to refresh the reference is provided with CameraCache.Refresh(camera). On the first call where the cached camera is null, Refresh(Camera.main) is called to initialize. Should I add this to documentation somewhere? What do you think of the names?

I moved MathUtils.GetHorizontalFieldOfViewRadians and MathUtils.IsInFov to a new CameraExtensions since they were both relying on and using Camera.main. Both Methods are not marked as obsolete and use the new extension with the cached camera instead. There was another method in Utils.MoveObjectInFrontOfUser that also uses the camera but I didn't move that one into Camera because I thought it didn't fit as well. But maybe camera.MoveObjectInFront might not be so bad.

Most scripts were simple replacements from Camera.main to CameraCache.main where I also added a local variable in some cases where it's used more than once.

2 Scripts cache the reference to the camera once on start: ScaleByDistance, MoveWithObject. Not sure how to deal with those. Maybe do it like the GazeManager that checks for the GazeTransform on every update?

Lastly GestureInteractiveData does this:

 protected Camera MainCamera { get { return Camera.main; } }

I changed it to use CameraCache but wasn't comfortable to just completely remove it and use local variables where it's used.

Camera.main calls a FindByTag on the whole scene to return the reference everytime. Cache the reference to the main camera instead and provide a method to overwrite it if necessary

Both GetHorizontalFieldOfViewRadians and IsInFov use Camera.main to get the reference to the main camera in their calculations. Set the old methods obsolte and move them into new camera extensions

Most scripts are simple replacement. Some scripts cache the reference themselves: ScaleByDistance, MoveWithObject

msftclas · 2017-08-12T17:16:04Z

@Zod-,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

jessemcculloch · 2017-08-12T17:29:43Z

Assets/HoloToolkit/Utilities/Scripts/CameraCache.cs

+        /// </summary>
+        /// <param name="newMain">New main camera to cache</param>
+        /// <returns></returns>
+        private static Camera CacheMain(Camera newMain)


The class name already tells us its the CameraCache, so maybe name this method something like SetMain

I realized the same and renamed it in a later commit to CameraCache.Refresh(camera). How do you like that one? Maybe another suggestion?
(I also totally forgot to change the automatically generated private to public which i fixed in the same commit)

StephenHodgson

Looking good!

I noticed there's places where we are caching our cached camera, when we can now just use a direct call to the CachedCamera.main

StephenHodgson · 2017-08-12T17:22:31Z

Assets/HoloToolkit/Utilities/Scripts/Extensions/CameraExtensions.cs

+        public static bool IsInFOV(this Camera camera, Vector3 position)
+        {
+            float verticalFovHalf = camera.fieldOfView / 2;
+            float horizontalFovHalf = camera.GetHorizontalFieldOfViewRadians() * Mathf.Rad2Deg / 2;


Nit: * 0.5f

Changed this, also added it to GetHorizontalFieldOfViewRadians

StephenHodgson · 2017-08-12T17:28:05Z

Assets/HoloToolkit/Utilities/Scripts/HeadsUpDirectionIndicator.cs

@@ -100,16 +101,17 @@ private void Update()
            }
            else
            {
+                Camera mainCamera = CameraCache.main;


~~Why cache again?~~

Not really for caching but I'm just used to have a local reference when I use something more than once. Using the full call to reference it every time and sometimes even twice in the same line can make the code quite a bit longer. I could change it back in cases where it's not as bad.

IMO calling it directly actually helps legibility, even if it's just a bit longer. Less Spaghetti.
Not that performance is an issue here, but local references do add to the heap (albeit negligible now days). Not a big deal.

I'll change it back in cases like this where there are fewer calls

StephenHodgson · 2017-08-12T17:30:42Z

Assets/HoloToolkit/Utilities/Scripts/CameraCache.cs

+        /// <summary>
+        /// Returns a cached reference to the main camera and uses Camera.main if it hasn't been cached yet.
+        /// </summary>
+        public static Camera main


Nit: public variables should be PascalCase.

Although this is very optional because it's following the same pattern as unity to get the main camera. Less work to refactor, but maybe making it PascalCase will show there's a difference between usages?

I had the same in mind to keep it the same as the unity call. But that's a good point to see a larger difference. One could mistake CameraCache.main for Camera.main when skimming over the code.

Changed it to PascalCase

/2 > 0.5f

Altough it seemed to make sense first to keep it the same as the unity api but to make a clearer difference between the two rename: main > Main.

StephenHodgson · 2017-08-12T19:21:10Z

Assets/HoloToolkit/Utilities/Scripts/HeadsUpDirectionIndicator.cs

@@ -61,7 +61,8 @@ private enum FrustumPlanes

        private void Start()
        {
-            Depth = Mathf.Clamp(Depth, Camera.main.nearClipPlane, Camera.main.farClipPlane);
+            Camera mainCamera = CameraCache.main;


~~We cache here, then again on line 104.~~

Oh I see, it's needed because it's the update. Please disregard comments.

StephenHodgson

Thanks!

Zod- · 2017-08-12T19:31:21Z

Should I still change the local variable caches or is it ok?

StephenHodgson · 2017-08-12T19:35:01Z

I think it's okay, not a big deal. I'll leave that up to you.

StephenHodgson · 2017-08-12T20:38:54Z

Assets/HoloToolkit/Utilities/Scripts/HeadsUpDirectionIndicator.cs

@@ -93,26 +92,23 @@ private void Start()
        // Update the direction indicator's position and orientation every frame.
        private void Update()
        {
-            // No object to track?
-            if (TargetObject == null || pointer == null)
+            if (!HasObjectsToTrack()) return;


nit braces { return; }

jessemcculloch · 2017-08-14T00:47:32Z

Assets/HoloToolkit/Utilities/Scripts/CameraCache.cs

@@ -0,0 +1,30 @@
+using UnityEngine;
+
+namespace HoloToolkit.Unity


Should the CameraCache be in the HoloToolkit.Unity.Input namespace? That is where our other camera related stuff lives right now.

I think it's fine, and the class should also include the license info in header.

StephenHodgson · 2017-08-14T02:32:21Z

Assets/HoloToolkit/Utilities/Scripts/Extensions/CameraExtensions.cs

@@ -0,0 +1,36 @@
+using UnityEngine;


nit License header

jessemcculloch

Thanks for the changes. Approved

keveleigh · 2017-08-15T22:06:46Z

This PR is giving me errors and appears to have broken GazeManager:

StephenHodgson · 2017-08-15T22:17:08Z

@keveleigh, same here. Workaround I found is assigning the gaze transform manually. Use the camera prefab you've got in your scene.

jessemcculloch · 2017-08-15T22:17:28Z

I'm not getting any errors. How are you guys testing?

Nevermind, There it showed up

StephenHodgson · 2017-08-15T22:25:24Z

Could be 2017.2 specific?

keveleigh · 2017-08-15T22:27:25Z

@StephenHodgson Sweet, thanks for the workaround!

@jessemcculloch I pulled master, loaded in Unity 2017.1.0f3, opened the InputTapTest scene, and hit the Play button.

Zod- · 2017-08-15T22:30:25Z

For some reason it doesn't seem to like GazeTransform = GazeTransform ?? CameraCache.Main.transform
Writing the full statement somehow works:

            if (GazeTransform == null)
            {
                GazeTransform = CameraCache.Main.transform;
            }
            else
            {
                GazeTransform = GazeTransform;
            }

StephenHodgson · 2017-08-15T22:31:56Z

Sometimes null coalescing doesn't work for components in Unity for some reason. It's hit or miss.

I think it's because even when a field is null, Unity still assigns an object that's still technically null to Monobehaviours, but not null in memory.

keveleigh · 2017-08-15T22:34:18Z

@Zod- I was just writing up the same finding :) Unity overrides == for GameObjects, which doesn't appear to play nicely with ??. From that blog post:

It behaves inconsistently with the ?? operator, which also does a null check, but that one does a pure c# null check, and cannot be bypassed to call our custom null check.

Zod- · 2017-08-15T22:41:11Z

Sorry about that I've fixed it in #864

Zod- added 4 commits August 12, 2017 17:40

Create CameraCache utility class

43797da

Camera.main calls a FindByTag on the whole scene to return the reference everytime. Cache the reference to the main camera instead and provide a method to overwrite it if necessary

Move camera related MathUtils in new extension

9d9eb4c

Both GetHorizontalFieldOfViewRadians and IsInFov use Camera.main to get the reference to the main camera in their calculations. Set the old methods obsolte and move them into new camera extensions

Rename cache refresh and make public

0f7ed9f

Use CameraCache.main rather than Camera.main

f4a8279

Most scripts are simple replacement. Some scripts cache the reference themselves: ScaleByDistance, MoveWithObject

msftclas added the cla-already-signed label Aug 12, 2017

jessemcculloch reviewed Aug 12, 2017

View reviewed changes

StephenHodgson suggested changes Aug 12, 2017

View reviewed changes

Zod- added 2 commits August 12, 2017 20:44

Use floats across the camera calculations

afd4740

/2 > 0.5f

Use PascalCase to provide a clearer difference

526d274

Altough it seemed to make sense first to keep it the same as the unity api but to make a clearer difference between the two rename: main > Main.

StephenHodgson reviewed Aug 12, 2017

View reviewed changes

StephenHodgson approved these changes Aug 12, 2017

View reviewed changes

Use less local caches where only 2 refs are used

cdc3c1e

StephenHodgson reviewed Aug 12, 2017

View reviewed changes

Add braces to inline return

324ef5d

jessemcculloch reviewed Aug 14, 2017

View reviewed changes

StephenHodgson reviewed Aug 14, 2017

View reviewed changes

Zod- added 2 commits August 14, 2017 10:05

Add license to CameraCache and CameraExtensions

cb709a0

Add class description on CameraCache

419dd32

jessemcculloch approved these changes Aug 15, 2017

View reviewed changes

jessemcculloch merged commit 4451dcb into microsoft:master Aug 15, 2017

StephenHodgson mentioned this pull request Aug 15, 2017

Revert "Implement camera cache utility class" #863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement camera cache utility class #852

Implement camera cache utility class #852

Zod- commented Aug 12, 2017 •

edited by StephenHodgson

msftclas commented Aug 12, 2017

jessemcculloch Aug 12, 2017

Zod- Aug 12, 2017

StephenHodgson left a comment

StephenHodgson Aug 12, 2017

Zod- Aug 12, 2017

StephenHodgson Aug 12, 2017 •

edited

Zod- Aug 12, 2017

StephenHodgson Aug 12, 2017

Zod- Aug 12, 2017

StephenHodgson Aug 12, 2017

StephenHodgson Aug 12, 2017 •

edited

Zod- Aug 12, 2017

Zod- Aug 12, 2017

StephenHodgson Aug 12, 2017 •

edited

StephenHodgson Aug 12, 2017

StephenHodgson left a comment

Zod- commented Aug 12, 2017

StephenHodgson commented Aug 12, 2017

StephenHodgson Aug 12, 2017

Zod- Aug 12, 2017

jessemcculloch Aug 14, 2017

StephenHodgson Aug 14, 2017 •

edited

StephenHodgson Aug 14, 2017

jessemcculloch left a comment

keveleigh commented Aug 15, 2017

StephenHodgson commented Aug 15, 2017

jessemcculloch commented Aug 15, 2017 •

edited

StephenHodgson commented Aug 15, 2017

keveleigh commented Aug 15, 2017

Zod- commented Aug 15, 2017

StephenHodgson commented Aug 15, 2017 •

edited

keveleigh commented Aug 15, 2017

Zod- commented Aug 15, 2017

		@@ -0,0 +1,30 @@
		using UnityEngine;

		namespace HoloToolkit.Unity

Implement camera cache utility class #852

Implement camera cache utility class #852

Conversation

Zod- commented Aug 12, 2017 • edited by StephenHodgson

msftclas commented Aug 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Aug 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Aug 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Aug 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson left a comment

Choose a reason for hiding this comment

Zod- commented Aug 12, 2017

StephenHodgson commented Aug 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenHodgson Aug 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessemcculloch left a comment

Choose a reason for hiding this comment

keveleigh commented Aug 15, 2017

StephenHodgson commented Aug 15, 2017

jessemcculloch commented Aug 15, 2017 • edited

StephenHodgson commented Aug 15, 2017

keveleigh commented Aug 15, 2017

Zod- commented Aug 15, 2017

StephenHodgson commented Aug 15, 2017 • edited

keveleigh commented Aug 15, 2017

Zod- commented Aug 15, 2017

Zod- commented Aug 12, 2017 •

edited by StephenHodgson

StephenHodgson Aug 12, 2017 •

edited

StephenHodgson Aug 12, 2017 •

edited

StephenHodgson Aug 12, 2017 •

edited

StephenHodgson Aug 14, 2017 •

edited

jessemcculloch commented Aug 15, 2017 •

edited

StephenHodgson commented Aug 15, 2017 •

edited