Add new warning UI for missing debug shaders and wireframe support. #5458

arttu-peltonen · 2021-08-27T10:53:52Z

EDIT: See Update for new screenshots

Purpose of this PR

As described by case 1342315, wireframe mode is not supported on some platforms and graphics APIs (at least Android GLES). This is a limitation with no easy fix, and Unity has not supported wireframes on these platforms in the past (see mention in docs).

Therefore, on these platforms, we display a warning that wireframe might not work. The capability is not exposed to C# so this is the best we can do right now, in the absence of a new C# API.

A note of this behavior should also be added to the documentation when that is written (cc @oleks-k).

Editor:

Runtime:

Testing status

Checked that the message apppears as expected on Windows Vulkan, but not when I switch to Windows DX11.

@evelinastaniulyte are you able to verify if wireframe works correctly on iOS? The bug only mentions Android but I'd like to verify iOS too (both Metal and GLES, if we still support that).

github-actions · 2021-08-27T10:54:07Z

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_trunk

URP
/.yamato%252Fall-urp.yml%2523PR_URP_trunk

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_trunk
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.

phi-lira

LGTM. It's missing changelog and I have one question about the way we check for platforms but pre-approving it as I will be OoO next week and don't want to block this one.

phi-lira · 2021-08-27T12:07:34Z

com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsRendering.cs

+                nameAndTooltip = Strings.WireframeNotSupportedWarning,
+                isHiddenCallback = () =>
+                {
+                    switch (SystemInfo.graphicsDeviceType)


I wonder if we should also get the the graphics APIs in the build settings, that way user might know that when he builds player to some of the platforms the runtime debug might not work? f.ex similar case here: https://sourcegraph.com/github.com/Unity-Technologies/Graphics/-/blob/com.unity.render-pipelines.universal/Editor/RendererFeatures/DecalRendererFeatureEditor.cs?L74-77

That's a good idea. I believe Editor itself only runs on platforms that support wireframe rendering, so build target platform is more relevant when user sees this warning in the editor.

I ended up doing something a bit different here. Instead of looking at build targets, I only show this warning in player builds. AFAIK this is never needed in the editor, only Android Vulkan/GLES have this problem. I think it's better that way you only really care about this information when you are using the debug feature on the device where it might not work correctly.

oleks-k

Approving with the edit.

arttupeltonen · 2021-08-30T06:27:13Z

Regarding Felipe's comment on changelog, I don't believe one is needed because rendering debugger is a new feature on URP.

ernestasKupciunas

Approving without testing. Good job with the PR description, all the images, and testing status!
Code seems to be relatively straightforward and shouldn't cause any issues on the URP side.
This PR will be tested manually on mobiles by other QA.

evelinastaniulyte

The warning message is hardly noticeable since missing contrast in the UI. It looks like plain text.
The warning text could be set to orange color, or at least the "Warning" word could be colored or bolded.

arttu-peltonen · 2021-08-31T10:44:46Z

The warning message is hardly noticeable since missing contrast in the UI. It looks like plain text.
The warning text could be set to orange color, or at least the "Warning" word could be colored or bolded.

@TheoWong-pixel what do you think? Might make sense, though the warning message will still look a bit clunky. Might be worth adding better support for warning labels such as this one...

TheoWong-pixel · 2021-08-31T10:56:33Z

Yeah this one should be an Info box. and the tooltip should be the message inside. :)

its fine for the runtime one, maybe if we can underline it, that would be a bonus. but for the in editor one, it should be an info box.

- Use it to display "wireframe unsupported" & "missing shaders" warnings in player builds.

- Also don't use deprecated field on URP.

arttu-peltonen · 2021-09-07T12:14:15Z

Update

Added new DebugUI.MessageBox widget type to allow displaying info/warning/error style messages.
- Ideally, this kind of UI would be tied into the widgets themselves but with current architecture I thought it easier to just make them separate widgets, and control their visibility as usual using isHiddenCallback.
- Editor
- Runtime
Using the new MessageBox warning for the "wireframe may not be supported" message. It is only shown in player builds (Android is the only known platform to not support this).
Added a new warning "Debug shaders missing" to clarify situations where user opens the debug UI and most modes are seemingly broken.
- URP
- HDRP
- Warning is only visible in player builds because shaders aren't stripped in the editor.
- Warning is not shown on "Display Stats", "Volume" or "Camera" tabs, because they don't require debug shaders.

oleks-k

Looks good.

ernestasKupciunas

Looks good. Warnings are clear and informative enough. Manually tested using URP Template scene on mobiles.

evelinastaniulyte

Looks good to me. Warnings are informative and the orange info box attracts attention, so I see no issues from UI/UX perspective.

# Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md

…5458

Add warning on Vulkan and GLES that wireframe may not be supported.

7763bb7

arttu-peltonen marked this pull request as ready for review August 27, 2021 10:54

arttu-peltonen requested review from a team as code owners August 27, 2021 10:54

github-actions bot added the universal label Aug 27, 2021

arttu-peltonen requested review from TheoWong-pixel, evelinastaniulyte and phi-lira August 27, 2021 10:54

phi-lira approved these changes Aug 27, 2021

View reviewed changes

Edited the warning and the tooltip.

a4b99cb

oleks-k approved these changes Aug 27, 2021

View reviewed changes

ernestasKupciunas self-requested a review August 30, 2021 09:00

ernestasKupciunas approved these changes Aug 30, 2021

View reviewed changes

evelinastaniulyte requested changes Aug 31, 2021

View reviewed changes

arttu-peltonen requested review from evelinastaniulyte and removed request for evelinastaniulyte September 1, 2021 08:09

arttu-peltonen added 4 commits September 7, 2021 11:30

Add new MessageBox DebugUI widget.

7f9e8d8

- Use it to display "wireframe unsupported" & "missing shaders" warnings in player builds.

Merge branch 'master' into srp/debug-wireframe-unsupported-warning

52e165a

Update visual style based on feedback.

724a454

Add missing debug shader warning for HDRP.

88f6fc2

- Also don't use deprecated field on URP.

arttu-peltonen self-assigned this Sep 7, 2021

arttu-peltonen added HDRP needs-backport-2021.2 SRP labels Sep 7, 2021

arttu-peltonen requested a review from oleks-k September 7, 2021 12:21

arttu-peltonen requested review from JulienIgnace-Unity, ernestasKupciunas, evelinastaniulyte and phi-lira September 7, 2021 12:21

arttu-peltonen changed the title ~~Add warning on Vulkan and GLES that wireframe may not be supported.~~ Add new warning UI for missing debug shaders and wireframe support. Sep 7, 2021

Changelog for Core & HDRP

fcc6ca3

TheoWong-pixel approved these changes Sep 7, 2021

View reviewed changes

arttu-peltonen requested review from evelinastaniulyte and removed request for evelinastaniulyte September 7, 2021 13:28

JulienIgnace-Unity approved these changes Sep 8, 2021

View reviewed changes

phi-lira approved these changes Sep 9, 2021

View reviewed changes

arttu-peltonen requested a review from a team September 13, 2021 05:51

sebastienlagarde approved these changes Sep 16, 2021

View reviewed changes

sebastienlagarde added 2 commits September 20, 2021 12:54

Merge branch 'master' into srp/debug-wireframe-unsupported-warning

4a200c6

Update CHANGELOG.md

7b38470

sebastienlagarde approved these changes Sep 20, 2021

View reviewed changes

oleks-k added 2 commits September 20, 2021 13:12

Edited the warning (URP)

dcc9f0a

Edited the warning (HDRP)

b039569

oleks-k approved these changes Sep 20, 2021

View reviewed changes

ernestasKupciunas approved these changes Sep 22, 2021

View reviewed changes

evelinastaniulyte approved these changes Sep 23, 2021

View reviewed changes

arttu-peltonen and others added 2 commits September 27, 2021 10:03

Merge branch 'master' into srp/debug-wireframe-unsupported-warning

e96adee

# Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md

Merge branch 'master' into srp/debug-wireframe-unsupported-warning

7acc0f7

sebastienlagarde merged commit 737a436 into master Sep 27, 2021

sebastienlagarde deleted the srp/debug-wireframe-unsupported-warning branch September 27, 2021 17:02

sebastienlagarde added backported-2021.2 and removed needs-backport-2021.2 labels Sep 27, 2021

sebastienlagarde pushed a commit that referenced this pull request Sep 27, 2021

Add new warning UI for missing debug shaders and wireframe support. #…

8ed200a

…5458

Add new warning UI for missing debug shaders and wireframe support. #5458

Add new warning UI for missing debug shaders and wireframe support. #5458

Uh oh!

Conversation

arttu-peltonen commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

EDIT: See Update for new screenshots

Purpose of this PR

Testing status

Uh oh!

github-actions bot commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phi-lira left a comment

Choose a reason for hiding this comment

Uh oh!

phi-lira Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

arttu-peltonen Aug 30, 2021

Choose a reason for hiding this comment

Uh oh!

arttu-peltonen Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

oleks-k left a comment

Choose a reason for hiding this comment

Uh oh!

arttupeltonen commented Aug 30, 2021

Uh oh!

ernestasKupciunas left a comment

Choose a reason for hiding this comment

Uh oh!

evelinastaniulyte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arttu-peltonen commented Aug 31, 2021

Uh oh!

TheoWong-pixel commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arttu-peltonen commented Sep 7, 2021

Uh oh!

oleks-k left a comment

Choose a reason for hiding this comment

Uh oh!

ernestasKupciunas left a comment

Choose a reason for hiding this comment

Uh oh!

evelinastaniulyte left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

arttu-peltonen commented Aug 27, 2021 •

edited

Loading

github-actions bot commented Aug 27, 2021 •

edited

Loading

evelinastaniulyte left a comment •

edited

Loading

TheoWong-pixel commented Aug 31, 2021 •

edited

Loading