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

Add API to completely disable Runtime Debug UI #5339

Merged
merged 7 commits into from
Sep 1, 2021

Conversation

arttu-peltonen
Copy link
Contributor

@arttu-peltonen arttu-peltonen commented Aug 12, 2021

Purpose of this PR

This PR is intended to mitigate issues raised in this forum thread: https://forum.unity.com/threads/renderpiplines-debugupdater-creates-duplicate-eventsystem-and-more.1154456/

Problem:

In order to support touch & mouse for Runtime Debug UI, we need to have EventSystem & corresponding *InputModule components present in the scene. To make this work out-of-the-box, DebugUpdater has been adding them automatically at startup, unless already present. This approach is problematic because some scenes (splash screen) might not have these components, but a subsequent scene would. Unity throws an error when more than one of these components are present, and this is what users are seeing.

Solution:

This PR makes it possible to disable DebugUpdater altogether using DebugManager.enableRuntimeUI (which replaces the enableWindowHotkey variable added in an earlier PR, meant to solve input conflicts). With this public API, users will be able to write scripts to initially disable the runtime UI, and also re-enable it later (at which point the UI will reuse existing EventSystem & *InputModule components if present).

This is not a perfect solution because some users will have to write custom scripts to make the runtime UI compatible with their game. The problem could be avoided if it were possible to have uGUI input (keyboard, touch, mouse, gamepad...) work without having to instantiate these components to the scene. AFAIK this is not currently possible (see slack discussion), so this is the best I have come up with.


Testing status

I used this test script in editor playmode to simulate users changing enableRuntimeUI value back and forth from script at runtime. Seems to work correctly - runtime UI disappears when value is disabled, and can be reopened when it is enabled again. Also when the value is disabled in Awake(), the DebugUpdater GameObject is never instantiated in the first place, thus avoiding any potential initialization overhead.

Additionally, tested that Debug UI & input still works as expected with these projects & platforms:

  • HDRP Sample (Windows) - Editor Playmode, both with Input Manager (Old) and Input System Package (New)
  • URP Sample (Windows) - both in Editor Playmode & Player build, both with Input Manager (Old) and Input System Package (New)
  • URP Sample (Android) - Player build, both with Input Manager (Old) and Input System Package (new)

I would appreciate some testing with "real projects" that have multiple scenes (ideally ones using input and others not using it), to see that Runtime Debug UI behaves sensibly when scene is changed.

…I that disables DebugUpdater entirely.

- When this property is set, all scene components created by DebugUpdater are either instantiated or destroyed.
- Also fixed a bug where debug actions were only enabled during the first time user enters playmode (when using new InputSystem).
- Moved previous changelog message from HDRP to Core since that's where the changes are.
@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/.yamato%252F_abv.yml%2523all_project_ci_2021.2
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@arttu-peltonen arttu-peltonen changed the title Srp/allow disabling runtime debug UI Add API to completely disable Runtime Debug UI to avoid EventSystem conflicts Aug 13, 2021
@arttu-peltonen arttu-peltonen changed the title Add API to completely disable Runtime Debug UI to avoid EventSystem conflicts Add API to completely disable Runtime Debug UI Aug 13, 2021
@RemyUnity RemyUnity requested a review from a team August 13, 2021 12:12
Copy link
Contributor

@alex-vazquez alex-vazquez left a comment

Choose a reason for hiding this comment

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

Minor comment about the docs

…er-Pipeline-Debug-Window.md

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
Copy link
Collaborator

@JulienIgnace-Unity JulienIgnace-Unity left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Might be good to add a small section to the what's new page to point to the new doc for more information regarding this new option.

@sebastienlagarde sebastienlagarde requested review from a team and removed request for a team August 25, 2021 12:05
Copy link
Contributor

@phi-lira phi-lira left a comment

Choose a reason for hiding this comment

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

LGTM but it's not really addressing the issues in the forum thread .
If there's no alternative not not rely on the components, what if we enable/disable the DebugUpdater upon specific times, such as scene loading/unloading?

@@ -16,10 +16,12 @@ The Render Pipeline Debugger window.

## Using the Render Pipeline Debugger window

To open the Render Pipeline Debugger window in the Editor, go to **Window > Analysis > Rendering Debugger**. You can also open this window at runtime in Play Mode, or in the standalone Unity Player on any device on **Development build**. Use the keyboard shortcut Ctrl+Backspace (Ctrl+Delete on macOS) or press L3 and R3 (Left Stick and Right Stick) on a controller to open the window. You can disable the shortcut through the [enableWindowHotkey variable](https://docs.unity3d.com/Packages/com.unity.render-pipelines.core@12.0/api/UnityEngine.Rendering.DebugManager.html#UnityEngine_Rendering_DebugManager_enableWindowHotkey).
To open the Render Pipeline Debugger window in the Editor, go to **Window > Analysis > Rendering Debugger**. You can also open this window at runtime in Play Mode, or in the standalone Unity Player on any device on **Development build**. Use the keyboard shortcut Ctrl+Backspace (Ctrl+Delete on macOS) or press L3 and R3 (Left Stick and Right Stick) on a controller to open the window.
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of information seems useful to have in the core part of the docs as it's common to both pipelines. It should be mentioned in mobile how you can enabled the window at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Right now, all Rendering Debugger docs are under HDRP as the URP docs are not yet written. I'd like to leave it to @oleks-k to figure out if/how to split the shared "Core" parts of the documentation, and keep HDRP/URP-specific parts under their respective packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now we don't have an efficient way to reuse the doc content between packages.
I'll copy this into to URP, and after 2021.2 will think of a way to reuse content (this is a tooling limitation).

Copy link
Contributor

@oleks-k oleks-k left a comment

Choose a reason for hiding this comment

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

I've edited the description and approving the PR now.

Copy link

Choose a reason for hiding this comment

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

The changes work fine. Tested with boat attack and changing scenes with/without eventsystem/input.

@arttu-peltonen arttu-peltonen removed the request for review from a team August 31, 2021 12:30
Copy link
Contributor

@Verasl Verasl left a comment

Choose a reason for hiding this comment

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

LGTM, a much needed improvement though it definitely still needs an automated solution in the near future as users will still encounter the issue still unless they add the extra code and systems to disable it thus will still feel compelled to send in bug reports.

oleks-k and others added 2 commits August 31, 2021 16:57
# Conflicts:
#	com.unity.render-pipelines.high-definition/Documentation~/Render-Pipeline-Debug-Window.md
@arttu-peltonen arttu-peltonen marked this pull request as ready for review August 31, 2021 17:05
@RemyUnity
Copy link
Contributor

Looks like @Unity-Technologies/gfx-qa-hdrp review is not needed anymore, but I still tested this PR with the spaceship demo project using this script .
I was able to reproduce the behavior described in the description, LGTM 👍

@RemyUnity RemyUnity self-requested a review August 31, 2021 17:21
@arttu-peltonen arttu-peltonen merged commit 4d09a54 into master Sep 1, 2021
@arttu-peltonen arttu-peltonen deleted the srp/allow-disabling-runtime-debug-ui branch September 1, 2021 05:56
arttu-peltonen added a commit that referenced this pull request Oct 12, 2021
* Add DebugManager.enableRuntimeUI that disables DebugUpdater entirely.

The code structure has changed quite a bit so this backport only includes the essential part required to fix case 1345783.
arttu-peltonen added a commit that referenced this pull request Oct 12, 2021
* Add DebugManager.enableRuntimeUI that disables DebugUpdater entirely.

The code structure has changed quite a bit so this backport only includes the essential part required to fix case 1345783.
sebastienlagarde pushed a commit that referenced this pull request Oct 15, 2021
* Add DebugManager.enableRuntimeUI that disables DebugUpdater entirely.

The code structure has changed quite a bit so this backport only includes the essential part required to fix case 1345783.
sebastienlagarde pushed a commit that referenced this pull request Oct 19, 2021
* Add DebugManager.enableRuntimeUI that disables DebugUpdater entirely.

The code structure has changed quite a bit so this backport only includes the essential part required to fix case 1345783.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants