Skip to content

Project settings UI #23

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.

Sign up for GitHub

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

Merged
mark-beiline merged 2 commits into from
Apr 4, 2023
Merged

Project settings UI #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9031e4
First pass
mark-beiline Apr 3, 2023
5da05d7
Removed unused code
mark-beiline Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions DisguiseUnityRenderStream/Editor/DisguiseRenderStreamSettingsEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
using UnityEditor;
using UnityEditor.UIElements;
using UnityEngine;
using UnityEngine.UIElements;

namespace Disguise.RenderStream
{
[CustomEditor(typeof(DisguiseRenderStreamSettings))]
class DisguiseRenderStreamSettingsEditor : Editor
{
public static class Style
{
public const string DisguiseSettingsContainer = "disguise-settings-container";
public const string DisguiseSettingsTitle = "disguise-settings-title";
}

[SerializeField]
VisualTreeAsset m_Layout;

[SerializeField]
StyleSheet m_Style;

public override VisualElement CreateInspectorGUI()
{
var root = new VisualElement();

m_Layout.CloneTree(root);
root.styleSheets.Add(m_Style);

root.Bind(serializedObject);

return root;
}
}
}
13 changes: 13 additions & 0 deletions DisguiseUnityRenderStream/Editor/DisguiseRenderStreamSettingsEditor.cs.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions DisguiseUnityRenderStream/Editor/DisguiseRenderStreamSettingsEditor.uss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
.disguise-settings-container
{
margin-left: 9px;
margin-top: 1px;
}

.disguise-settings-title
{
font-size: 19px;
-unity-font-style: bold;
margin-bottom: 12px;
}
11 changes: 11 additions & 0 deletions DisguiseUnityRenderStream/Editor/DisguiseRenderStreamSettingsEditor.uss.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions DisguiseUnityRenderStream/Editor/DisguiseRenderStreamSettingsEditor.uxml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<ui:UXML xmlns:ui="UnityEngine.UIElements" xmlns:uie="UnityEditor.UIElements">
<uie:PropertyField label="Scene Control" binding-path="sceneControl" />
Copy link
Copy Markdown
Owner

@wenzhang-unity wenzhang-unity Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Minor: In code, I've started using L10.Tr() for strings, because this was the recommendation I got on the UIToolkit slack. Unfortunately, they don't have a good answer for strings in UXML. Figures 😞

<uie:PropertyField label="Unity Debug Window Presenter" binding-path="enableUnityDebugWindowPresenter" />
</ui:UXML>
10 changes: 10 additions & 0 deletions DisguiseUnityRenderStream/Editor/DisguiseRenderStreamSettingsEditor.uxml.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 37 additions & 0 deletions DisguiseUnityRenderStream/Editor/DisguiseRenderStreamSettingsProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
using UnityEditor;
using UnityEngine.UIElements;

namespace Disguise.RenderStream
{
static class DisguiseRenderStreamSettingsProvider
{
static readonly string k_SettingsPath = "Project/DisguiseRenderStream";

class Contents
{
public const string SettingsName = "Disguise RenderStream";
}

[SettingsProvider]
static SettingsProvider CreateSettingsProvider() =>
new (k_SettingsPath, SettingsScope.Project)
{
label = Contents.SettingsName,
Copy link
Copy Markdown
Collaborator

@FrederickUnity FrederickUnity Apr 4, 2023

Choose a reason for hiding this comment

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

Food for thoughts: I noticed that when the Cluster Display package looks like is present settings looks like this:
image

I wonder, is it what we want? Because there are a lot of settings in there that are not for Disguise RenderStream and could potentially confuse the user (and cause trouble if wrongly configured). In fact, when Cluster Display is working with Disguise I'd be tempted to have the following settings:
Disguise RenderStream:

  • Scene Control
  • Unity Debug Window Presenter
  • Cluster Synchronization (as a "sub section", like Cluster Render and Mission Control is to Cluster Display)
    • Multicast Address
    • Port
    • Handhsake Timeout
    • Communication Timeout
    • Adapter Name

But I guess this cannot be achieved only in this package, we also need a way for the ClusterDisplay package to stop providing its own settings when "requested" (or maybe we simply want to hardcode checking for the DisguiseRenderStream package)

Copy link
Copy Markdown
Owner

@wenzhang-unity wenzhang-unity Apr 4, 2023

Choose a reason for hiding this comment

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

A few thoughts:

  1. Importing the Cluster Graphics package is a mistake for RenderStream projects (the "Cluster Rendering" section should not be present)
  2. I don't like the idea of hardcoding a check for the presence of DisguiseRenderStream in the Cluster Display logic. We can provide a link in the Disguise RenderStream settings page to the Cluster Display settings. Or we can expose some of the Cluster Display fields in the RenderStream settings, but this might add confusion.

Copy link
Copy Markdown
Collaborator

@FrederickUnity FrederickUnity Apr 4, 2023

Choose a reason for hiding this comment

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

Oups, my bad.

Yeah, I must admit I'm not a big fan of Cluster Display knowing about Disguise RenderStream package either. What about Cluster Display only adds its setting pages if Cluster Graphics package is present? Not perfect but "less worst"?

Copy link
Copy Markdown
Owner

@wenzhang-unity wenzhang-unity Apr 4, 2023

Choose a reason for hiding this comment

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

That's interesting. I rather like the idea of making Cluster Display (core) a "hidden" dependency of other packages (e.g. Graphics, RenderStream), kind of similar to Core SRP. Let's take a look at how Core SRP settings are exposed for inspiration.

activateHandler = (searchContext, parentElement) =>
{
var settings = DisguiseRenderStreamSettings.GetOrCreateSettings();
var editor = Editor.CreateEditor(settings);

var gui = editor.CreateInspectorGUI();
gui.AddToClassList(DisguiseRenderStreamSettingsEditor.Style.DisguiseSettingsContainer);

var title = new Label { text = Contents.SettingsName };
title.AddToClassList(DisguiseRenderStreamSettingsEditor.Style.DisguiseSettingsTitle);
gui.Insert(0, title);

parentElement.Add(gui);
},
keywords = SettingsProvider.GetSearchKeywordsFromSerializedObject(new SerializedObject(DisguiseRenderStreamSettings.GetOrCreateSettings()))
};
}
}
11 changes: 11 additions & 0 deletions DisguiseUnityRenderStream/Editor/DisguiseRenderStreamSettingsProvider.cs.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions DisguiseUnityRenderStream/Runtime/PipelineAbstraction/AssemblyInfo.cs.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions ...rStream/Runtime/PipelineAbstraction/Disguise.RenderStream.PipelineAbstraction.asmdef.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions DisguiseUnityRenderStream/Runtime/Util/AutoDisposable.cs.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.