EditorVR camera better represents what is seen in the game view by dunity · Pull Request #431 · Unity-Technologies/EditorXR

dunity · 2017-11-27T19:45:04Z

Purpose of this PR

This improvement allows for the EditorVR VRView camera to better represent what is seen in the game view.

Add support for copying the scene's MainCamera settings (clear flags, background, etc), if one exists, and the (EditorVR)context's m_CopySceneCameraSettings inspector bool/toggle is enabled.

Testing status

Tested in various scenes and camera settings scenarios. Tested with and without a MainCamera in the scene.

Technical risk

Medium - The majority of the changes are relegated to EditorVRContext, EditingContextManager, and VRView. These are core EVR elements; some of their fields/properties have been made static, in order to allow for this functionality to occur as/when needed. Care has been taken to avoid conflicting with the previous AddComponent approach. Disabling the m_CopySceneCameraSettings bool on the EditorVR context will disable this functionality, and create the camera in the manner that was previously occurring.

Notes

I've also added initial support for the Unity PostProcessing(v2) stack in the following branch: "improvements/dylanu/scene-camera-fx-settings". There is more work to be done in that branch before PR. This branch/PR acts as the foundation for that work.

… a scene MainCamera exists).

…used for latter re-setting in OnDisable

… OnCameraSetupStarted()

AndrewTHEManeri

Not on hardware so I can't test this out - what happens if you have a main camera that has more exotic settings - like set to deferred rendering?

AndrewTHEManeri · 2017-11-28T05:46:36Z


 namespace UnityEditor.Experimental.EditorVR.Core
 {
+    public struct ContextSettings


This should be in a separate file so that IEditingContext is not dependent on EditorVRContext

AndrewTHEManeri · 2017-11-28T05:47:35Z

+        /// <summary>
+        /// Settings denoting that the scene camera's (component_ values should be cloned on the XR runtime camera
+        /// </summary>
+        ContextSettings contextSettings { get; }


Why this context settings class instead of individual settings items being exposed as interface members/requirements?

I was initially trying to avoid adding the copySceneCameraSettings param to IEditingContext. The initial number of settings were more numerous when accounting for PostFX settings (coming in a latter PR). Though after I minimized the implementation, it now makes sense to merge it back into IEditingContext, and remove the ContextSettings struct (for now).

dunity · 2017-11-29T01:06:57Z

@AndrewTHEManeri These changes intentionally allow for copying of any camera settings that aren't replaced by those that are defined in VRView OnEnable(), including forward/deferred/etc. VRView still sets/replaces values for CameraType/OcclusionCulling/ClipPlanes. Ideally, we could also support an EVR "CameraCopy" component, that users could set on their desired camera. Such a component would have toggles allowing for individual copying of specific params, etc. On personal projects, I'm needing such camera settings copying.

… include copySceneCameraSettings property in IEditingContext; this is used in VRView OnEnable() when setting up the VRView camera

dunity · 2017-11-29T01:39:50Z

ContextSettings struct removed from IEditingContext & EditorVRContext. copySceneCameraSettings property added to IEditingContext. This is used in VRView OnEnable() when setting up the VRView camera.

dunity · 2017-11-29T06:26:43Z

@AndrewTHEManeri your suggested changes have been applied (as I understood them).

…ings-copy # Conflicts: # Scripts/Core/Contexts/EditingContextManager.cs # Scripts/Core/Contexts/EditorVR.asset # Tests/Editor/Unit/Core/EditingContextManagerTests.cs

…ntextManager post-dev-merge

mtschoen-unity

Just one change request, which is to use EditorUtility.CopySerialized in your CopyComponent method.

mtschoen-unity · 2017-12-05T19:01:50Z

            return component;
        }

+        public static T CopyComponent<T>(T sourceComponent, GameObject targetGameObject) where T : Component


Check out EditorUtility.CopySerialized. You should be able to just AddComponent, and then CopySerialized(source, target)

good call; I'm refactoring the method now

mtschoen-unity · 2017-12-05T19:03:07Z

-            m_Instance.Shutdown(); // Give a chance for dependent systems (e.g. serialization) to shut-down before destroying
-            ObjectUtils.Destroy(m_Instance.gameObject);
-            m_Instance = null;
+            if (m_Instance)


Why the null check?

I ran into a case that produced an exception when calling Dispose() before the EXR instance is created in IEditingContext's Setup() function. I've just pushed an alternate fix that validates that an instance has been created, via the instanceExists bool prop in IEditingContext, before attempting to dispose in EditingContextManager's SetEditingContext() function.

mtschoen-unity · 2017-12-05T19:04:34Z

-
-        List<IEditingContext> m_AvailableContexts;
-        string[] m_ContextNames = null;
+        string[] m_ContextNames;


Making all of this static is a little worrying to me, but I haven't found any issues in testing.

There's a need to access some context data before instantiation occurs. I ran a ton of tests too, and couldn't find any issues. If anyone wants to suggest, or craft a better solution, I'm open to any input/changes.

…ool prop in IEditingContext, before attempting to dispose in EditingContextManager's SetEditingContext() function

…ty.CopySerialized instead of custom logic

dunity · 2017-12-05T23:47:37Z

Suggested changes pushed. Please give it another look when you have the time @mtschoen-unity , thanks :-)

…ditorVR into improvements/dylanu/scene-camera-settings-copy

Fix issue where camera settings copy doesn't happen the first time you launch EXR after a domain reload

mtschoen-unity · 2017-12-08T03:35:02Z

Forgot to mention in my commit message that the last one also fixes an issue with the Editing Context popup in VRView if there is >1 context available. It was broken since the GUILaout->GUI refactor.

@AndrewTHEManeri I'm done making changes and this should be good to go. Still waiting on your approval.

jono-unity · 2017-12-09T01:29:06Z

@dunity here's some funky behavior: it looks like if 'Allow HDR' is false on a camera, the presentation camera gets squished. I'm seeing this in an empty (just camera and light) scene and in the Poly Pirates main scene, and not on dev. In this vid, I'm toggling 'presentation camera': https://youtu.be/Zpk_kMgKGCU

mtschoen-unity · 2017-12-09T02:42:00Z

@jono-unity --can confirm. Away it goes, then. I'll force HDR on like I did with the other settings that break things.

mtschoen-unity · 2017-12-09T02:45:38Z

@stella3d this feature is a perfect example for your AI monkey script. It could run EXR with all permutations of camera settings and record failures. 👍

mtschoen-unity · 2017-12-09T06:29:56Z

Manual merge successful. Performing Github merge

dunity added 4 commits November 26, 2017 17:45

Implement scene MainCamera settings copying for EXR VRView camera (if…

0cf9279

… a scene MainCamera exists).

Cache the enabled state of the existing scene main camera in VRView; …

c5e1cf9

…used for latter re-setting in OnDisable

Return the current context, if one exists, in EditingContextManager's…

9d56e4f

… OnCameraSetupStarted()

Fix unnecessary setting of existing camera enabled state in VRView

87e3781

dunity added the improvement label Nov 27, 2017

dunity self-assigned this Nov 27, 2017

dunity requested review from AndrewTHEManeri and mtschoen-unity November 27, 2017 19:45

AndrewTHEManeri suggested changes Nov 28, 2017

View reviewed changes

Remove ContextSettings struct from IEditingContext & EditorVRContext;…

86804d9

… include copySceneCameraSettings property in IEditingContext; this is used in VRView OnEnable() when setting up the VRView camera

dunity requested a review from stella3d November 29, 2017 23:43

dunity added 3 commits November 30, 2017 14:25

Merge branch 'development' into improvements/dylanu/scene-camera-sett…

476d71b

…ings-copy # Conflicts: # Scripts/Core/Contexts/EditingContextManager.cs # Scripts/Core/Contexts/EditorVR.asset # Tests/Editor/Unit/Core/EditingContextManagerTests.cs

Revive new default/static context fetching functionality in EditingCo…

dcd822a

…ntextManager post-dev-merge

Add nearClipPlane correction in VRView camera setup, in OnEnable()

21e7c2f

dunity removed the request for review from stella3d December 1, 2017 01:11

mtschoen-unity requested changes Dec 5, 2017

View reviewed changes

dunity added 2 commits December 5, 2017 15:31

Validate that an instance has been created, via the instanceExists b…

cfa2936

…ool prop in IEditingContext, before attempting to dispose in EditingContextManager's SetEditingContext() function

Refactor CopyComponent function to utilize addComponent & EditorUtili…

24c4406

…ty.CopySerialized instead of custom logic

mtschoen-unity approved these changes Dec 6, 2017

View reviewed changes

mtschoen-unity added 5 commits December 7, 2017 17:53

Fix line endings

c98e605

Merge branch 'development' of https://github.com/Unity-Technologies/E…

0601c5b

…ditorVR into improvements/dylanu/scene-camera-settings-copy

Fix line endings

aec0f73

Fix issue if scene camera clear flags are set to Nothing;

668367d

Fix issue where camera settings copy doesn't happen the first time you launch EXR after a domain reload

Force copied camera stereoTargetEye to Both

693c142

Force HDR on because of a bug in the mirror view

29e0cd1

mtschoen-unity merged commit e515e00 into development Dec 9, 2017

mtschoen-unity deleted the improvements/dylanu/scene-camera-settings-copy branch December 9, 2017 06:30

Uh oh!

Conversation

dunity commented Nov 27, 2017

Purpose of this PR

Testing status

Technical risk

Notes

Uh oh!

AndrewTHEManeri left a comment

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!

dunity commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunity commented Nov 29, 2017

Uh oh!

dunity commented Nov 29, 2017

Uh oh!

mtschoen-unity left a comment

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!

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!

dunity commented Dec 5, 2017

Uh oh!

mtschoen-unity commented Dec 8, 2017

Uh oh!

jono-unity commented Dec 9, 2017

Uh oh!

mtschoen-unity commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtschoen-unity commented Dec 9, 2017

Uh oh!

mtschoen-unity commented Dec 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dunity commented Nov 29, 2017 •

edited

Loading

mtschoen-unity commented Dec 9, 2017 •

edited

Loading