Zero-click presenter with texture selection dropdown by mark-beiline · Pull Request #22 · wenzhang-unity/RenderStream-Unity

mark-beiline · 2023-03-24T20:19:26Z

Project-wide setting enabled by default:

Adds remote parameter group to schema:

Options are None + channels + live textures:

A number of resize strategies:

…senter for all scenes)

mark-beiline · 2023-03-27T15:49:31Z

Supporting changes:

DisguiseRenderStream.cs: Callbacks and access to live textures.
BlitStrategy.cs: Added Clamp strategy
CameraCapturePresenter.cs: It was assumed that the CameraCapture component lived on the same GameObject. This was limiting in the case of the DisguisePresenter prefab, so now it supports both ways (auto-assign if null).
Presenter.cs: Moved away from Graphics.* API for clearing the screen (which blocks the main thread). Automatically clears the screen after being disabled to prevent a "frozen" frame.

…Unity into mark/present-to-screen-v3 # Conflicts: # DisguiseUnityRenderStream/Editor/DisguiseRenderStreamBuild.cs

…for "resizeStrategy" instead)

mark-beiline · 2023-03-27T19:21:34Z

        //      overshadows a parent member (ex TextMeshPro.renderer). Since we traverse the hierarchy top to bottom,
        //      the top-level member will be resolved.
-
+        // * BindingFlags.IgnoreCase: For secondary lookup (ex "Position" after "m_Position" failed we want to match both "Position" and "position").


The root of these edge cases is that we're trying to match UnityEditor.SerializedProperty.propertyPath to a System.Reflection.MemberInfo.

This could be fixed in another PR where we wouldn't use SerializedProperty anymore, by enumerating fields/properties using System.Serialization directly.

FrederickUnity · 2023-03-28T11:25:03Z

Is it possible you are missing the .meta for that prefab and DisguisePresenter.cs? I have a null reference exception while building and I think this is the cause.

I think this would be because the .gitignore has *.meta, do we want to keep that?

Added the .meta files. They're all "default" meta files, except for NativeRenderingPlugin.dll.meta which is configured for Windows x64. Which makes me think maybe the bug could be unrelated to meta files because your default meta files should work like mine.

Build exception is fixed. Probably the prefab uses some uuid from the .meta. So although it is the default one, my computer did not generate the same uuid than yours.

FrederickUnity · 2023-03-29T11:15:16Z

+
+namespace Disguise.RenderStream
+{
+    /// <summary>


Brainstorm: This is brilliant to understand when we have a problem I tried and love it! However does that have any other usage than debug? Will it be used in production? If no then:

Should we give a name making it more obvious that this is for debugging? UnityDebugPresenter? (Unity vs Disguise because the debug is displayed in the unity window on the rendering computer)

Do we want this debug to be present by default (should probably ask Kylo?)? I'm asking because generally debugging in software are off by default.

Good point. Any preference between UnityDebugPresenter and UnityWindowPresenter? I like "window" because it removes ambiguity around "local display" or "local screen".

The default texture selection is "None" so it's opt-in even when it's included in the schema. So the user wouldn't have to make a new build when they want to enable it.

Personally I like to have Debug in there so it is obvious that it is for debugging. What about UnityDebugWindowPresenter? The best of both world?

Is there potential draw back of selecting something? I think it has a small performance impact? What about user activate it, forget about it and then has worst performance than before and can't find why?

FrederickUnity · 2023-03-29T11:18:29Z

+        }
+
+        private const string k_PrefabPath = "DisguisePresenter";
+        private const string k_NoneTextureLabel = "None";


Minor from Rider: Inconsistent modifiers style: redundant 'private' modifier

FrederickUnity · 2023-03-29T11:33:13Z

+        /// The choices for the texture selection dropdown are scene-specific and correspond to a concatenated list of:
+        /// None + Channels (output) + Live textures (input).
+        /// </remarks>
+        public static List<ManagedRemoteParameter> GetManagedRemoteParameters(ManagedSchema schema, ManagedRemoteParameters sceneSchema)


Curiosity: Why do we have this special mechanism for those remote parameters? Can't they be simply picked-up as normal remote parameters?

I wanted to add the presenter to all scenes automatically. One method would be to add it to each scene in a build postprocessor but I'm not a fan of modifying scene files under the hood.

This other method is to add it to each scene at runtime after it's loaded. We only need to make sure it's in the schema by retrieving the presenter parameters from GetManagedRemoteParameters.

It also does some processing on the parameters, most importantly we need to collect the texture names for the dropdown (channels are global, but live textures are per-scene). So GetManagedRemoteParameters needs to be called after a first version of the schema has been generated.

FrederickUnity · 2023-03-29T11:42:58Z

Improvement idea: Instead of keeping those settings in some ScriptableObject hidden somewhere in the assets, should we add it to the project settings? Using [SettingsProvider] like Wen did in ClusterDisplay?

Agreed, I would expect to see them in Project Settings. I found the files you mentioned, I can do it in a follow-up PR.

FrederickUnity · 2023-03-29T11:44:56Z

+    /// The generated schema will include remote parameters to select the texture to display and how to resize it to fit the screen.
+    /// </summary>
+    [SerializeField]
+    public bool exposePresenter = true;


Minor: If we keep the default UI for those those two fields (which on first sight are sufficient), maybe we should add tooltips with parts of the summary for those two parameters?

wenzhang-unity

Awesome. I left some comments for you to consider.

wenzhang-unity · 2023-03-29T13:05:11Z

+    /// <para>
+    /// 2. Presenting a texture to the screen according to <see cref="Selected"/> and <see cref="ResizeStrategy"/>.
+    /// </para>
+    /// </summary>


Minor (style): The longer detailed description can go into <remarks>

wenzhang-unity · 2023-03-29T13:16:35Z

+        /// <remarks>
+        /// <paramref name="outputIdx"/> and <paramref name="inputIdx"/> are -1 when invalid.
+        /// </remarks>
+        void VirtualIndexToRealIndex(int virtualIdx, out int outputIdx, out int inputIdx)


A (very) inconsequential case here, but in general, instead of using magic numbers, prefer an alternative such as returning a bool or using nullables. This basically forces the user to deal with the "invalid" case (instead of accidentally passing -1 into an array). e.g.

(int? outputIdx, int? inputIdx) ToRealIndices(int virtualIdx)

Also, it might be more readable if this was split into 2 methods, since the 2 indices are calculated independently as far as I can tell.

wenzhang-unity · 2023-03-29T13:41:38Z

        {
+            if (DisguiseRenderStreamSettings.GetOrCreateSettings().exposePresenter)
+            {
+                GameObject.Instantiate(DisguisePresenter.LoadPrefab());


Curious: Why do we need a prefab? Why not create the components and set their fields via scripting? I can't really review a .prefab.

Yes the prefab is equivalent to

var root = new GameObject("Disguise Presenter"); var disguisePresenter = root.AddComponent<DisguisePresenter>(); var outputPresenterGO = new GameObject("Output Presenter"); outputPresenterGO.transform.parent = root.transform; var outputPresenter = outputPresenterGO.AddComponent<CameraCapturePresenter>(); var inputPresenterGO = new GameObject("Input Presenter"); inputPresenterGO.transform.parent = root.transform; var inputPresenter = inputPresenterGO.AddComponent<Presenter>(); disguisePresenter.OutputPresenter = outputPresenter; disguisePresenter.InputPresenter = inputPresenter;

But I can't add the remote parameters through code. They look like this in the prefab:

wenzhang-unity · 2023-03-29T13:44:54Z

+            get
+            {
+                if (LatestFrameData.scene > m_SceneFields.Length)
+                    return null;


I think it's less burdensome on the client if you return Enumerable.Empty<RenderTexture>() here instead of null, which will require special handling.

…lated ScriptableRenderContext + no reason to clear depth&stencil buffers)

mark-beiline added 5 commits March 22, 2023 14:42

First pass

ae99e72

Fix

a461157

Zero-click setup (project-wide setting to automatically setup the pre…

73daea1

…senter for all scenes)

Removed unused file

09d9c64

Dropdown of channel/texture names for Presenter

86a2dae

mark-beiline mentioned this pull request Mar 24, 2023

Zero-click presenter #21

Closed

mark-beiline added 2 commits March 24, 2023 16:36

Fix leftover frame on screen after turning off the presenter

f45d4f9

API docs

2e75a3e

mark-beiline marked this pull request as ready for review March 27, 2023 15:24

mark-beiline requested review from FrederickUnity and wenzhang-unity March 27, 2023 15:25

mark-beiline added 3 commits March 27, 2023 14:38

Merge branch 'dev' of https://github.com/wenzhang-unity/RenderStream-…

cdfb11d

…Unity into mark/present-to-screen-v3 # Conflicts: # DisguiseUnityRenderStream/Editor/DisguiseRenderStreamBuild.cs

Merge fix

ba46b70

Bugfix ("m_ResizeStrategy" not matching "ResizeStrategy" but looking …

6b00c3a

…for "resizeStrategy" instead)

mark-beiline commented Mar 27, 2023

View reviewed changes

FrederickUnity reviewed Mar 28, 2023

View reviewed changes

Added .meta files

fcda6dd

FrederickUnity reviewed Mar 29, 2023

View reviewed changes

wenzhang-unity requested changes Mar 29, 2023

View reviewed changes

mark-beiline added 3 commits March 29, 2023 13:53

PR suggestions

57b4340

Optimizing presenter (it was running twice a frame because of an unre…

3af84c1

…lated ScriptableRenderContext + no reason to clear depth&stencil buffers)

Renamed DisguisePresenter -> UnityDebugWindowPresenter

9b30f6f

FrederickUnity approved these changes Mar 30, 2023

View reviewed changes

wenzhang-unity approved these changes Mar 31, 2023

View reviewed changes

mark-beiline merged commit 59fd78a into dev Mar 31, 2023

mark-beiline deleted the mark/present-to-screen-v3 branch March 31, 2023 16:03

Conversation

mark-beiline commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mark-beiline commented Mar 27, 2023

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!

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!

Choose a reason for hiding this comment

Uh oh!

wenzhang-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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mark-beiline commented Mar 24, 2023 •

edited

Loading