Skip to content

Conversation

arttu-peltonen
Copy link
Contributor

@arttu-peltonen arttu-peltonen commented Sep 16, 2021

Purpose of this PR

This main point of this PR is to expose public DebugDisplaySettings API. This is useful for both internal and external users, who want to create custom UI for certain debug modes. This was the exact use case for Lookdev Studio team. This PR will bring URP to parity with HDRP, where an equivalent API is already public.

Public DebugDisplay API

Users can now control DebugDisplay from script. Example usage:

public void Update()
{
    var debugData = UniversalRenderPipelineDebugDisplaySettings.Instance;

    debugData.materialSettings.materialDebugMode = DebugMaterialMode.Albedo;
    debugData.lightingSettings.lightingDebugMode = DebugLightingMode.ShadowCascades;
    debugData.renderingSettings.wireframeMode = DebugWireframeMode.SolidWireframe;
}

The PR renames the now public properties to be consistent and use camelCase as described in coding standards.

Added AlbedoDebugValidationPreset.Custom

As an incidental change, a new preset has been added to Material Albedo validation presets: AlbedoDebugValidationPreset.Custom. This makes sense from API point of view, and also useful from user perspective: In the images above, the custom color has been set to yellowis-orange, which matches the albedo of helmet and power tool, and gives error colors for other albedos:

image
image


Testing status

Tested locally by creating a simple script (custom-debugui-v2.zip) that uses the public API. This simulates the usecase that users of the API would have.

custom-debug-ui

This should demonstrate that the API works as intended - feel free to download the attached script and try it out.

In terms of other testing, please do a quick test for rendering debugger to see that nothing has been broken by the renaming.

- Made the settings API public
- Renamed properties to be more uniform with each other and use camelCase
- Added new Material Albedo validation preset: AlbedoDebugValidationPreset.Custom
@github-actions
Copy link

github-actions bot commented Sep 16, 2021

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://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, 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
/jobDefinition/.yamato%2Fall-hdrp.yml%23HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

URP
/jobDefinition/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/jobDefinition/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

Shader Graph
/jobDefinition/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk
Depending on your PR, you may also want
/jobDefinition/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_trunk
/jobDefinition/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_trunk

VFX
/jobDefinition/.yamato%252Fall-vfx.yml%2523PR_VFX_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:
/jobDefinition/.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.

@arttu-peltonen arttu-peltonen marked this pull request as ready for review September 16, 2021 08:37
@arttu-peltonen arttu-peltonen requested review from a team as code owners September 16, 2021 08:37
…vidually.

- Changed DebugDisplaySettingsCommon interface implementation because of this. The common settings now report everything as "not active" because the widgets are not owned by it.
Copy link

Choose a reason for hiding this comment

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

Seems to be working well, didn't discover any issues withthe rendering debugger.

# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsCommon.cs
#	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsRendering.cs
#	com.unity.render-pipelines.universal/Runtime/Debug/UniversalRenderPipelineDebugDisplaySettings.cs
#	com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs
Comment on lines +109 to +111
public bool AreAnySettingsActive => false;
public bool IsPostProcessingAllowed => true;
public bool IsLightingActive => true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels a bit inconsistent that these properties are Upper Camel Case. But I guess this was already done before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the reason here was that I didn't want to go through deprecation process just to change the first letter to be lowercase, as this is implementing an interface that was already public (even though this & other implementing classes have been internal up until now). If you'd prefer consistency here, I can do it. What do you think?

Copy link
Contributor

@ellioman ellioman Oct 5, 2021

Choose a reason for hiding this comment

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

I personally think it's worth it as API consistency decreases the WTF! moment users will have.
If it's not too much trouble I would love to get that change but it can also be done in a followup PR.

@Verasl Verasl self-requested a review October 4, 2021 15:06
- Exposed public setters to albedoHueTolerance and albedoSaturationTolerance that were still private.
- Fixed compile error in player build due to renamed symbol.
@phi-lira phi-lira changed the base branch from master to 2021.2/staging October 12, 2021 09:44
@phi-lira phi-lira requested review from a team as code owners October 12, 2021 09:44
@phi-lira phi-lira changed the base branch from 2021.2/staging to universal/staging October 12, 2021 09:45
@phi-lira phi-lira removed request for a team October 12, 2021 09:46
@phi-lira phi-lira deleted the branch master October 12, 2021 14:07
@phi-lira phi-lira closed this Oct 12, 2021
@phi-lira phi-lira reopened this Oct 12, 2021
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
@arttu-peltonen arttu-peltonen requested a review from a team as a code owner October 13, 2021 07:55
@arttu-peltonen arttu-peltonen changed the base branch from universal/staging to master October 13, 2021 07:57
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

phi-lira added a commit that referenced this pull request Oct 20, 2021
commit c0c961d
Merge: f6e204f b8c4896
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Wed Oct 13 10:54:12 2021 +0300

    Merge branch 'master' into srp/urp-debugdisplay-api

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

commit f6e204f
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Tue Oct 5 12:06:17 2021 +0300

    Change public fields into public properties.

    - Exposed public setters to albedoHueTolerance and albedoSaturationTolerance that were still private.
    - Fixed compile error in player build due to renamed symbol.

commit 793e63f
Merge: 945724e f0c1049
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Mon Oct 4 16:09:35 2021 +0300

    Merge branch 'master' into srp/urp-debugdisplay-api

    # Conflicts:
    #	com.unity.render-pipelines.universal/CHANGELOG.md
    #	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsCommon.cs
    #	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsRendering.cs
    #	com.unity.render-pipelines.universal/Runtime/Debug/UniversalRenderPipelineDebugDisplaySettings.cs
    #	com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs

commit 945724e
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Thu Sep 16 14:26:26 2021 +0300

    Iterate m_Settings array instead of calling each settings object individually.

    - Changed DebugDisplaySettingsCommon interface implementation because of this. The common settings now report everything as "not active" because the widgets are not owned by it.

commit 17a8ef5
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Thu Sep 16 10:46:41 2021 +0300

    Fix casing to camelCase for settings properties

commit df48fa1
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Thu Sep 16 10:06:08 2021 +0300

    Changelog

commit 2971ab6
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Thu Sep 16 09:15:36 2021 +0300

    Public API for URP DebugDisplaySettings

    - Made the settings API public
    - Renamed properties to be more uniform with each other and use camelCase
    - Added new Material Albedo validation preset: AlbedoDebugValidationPreset.Custom
@phi-lira phi-lira mentioned this pull request Oct 20, 2021
phi-lira added a commit that referenced this pull request Oct 22, 2021
commit c0c961d
Merge: f6e204f b8c4896
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Wed Oct 13 10:54:12 2021 +0300

    Merge branch 'master' into srp/urp-debugdisplay-api

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

commit f6e204f
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Tue Oct 5 12:06:17 2021 +0300

    Change public fields into public properties.

    - Exposed public setters to albedoHueTolerance and albedoSaturationTolerance that were still private.
    - Fixed compile error in player build due to renamed symbol.

commit 793e63f
Merge: 945724e f0c1049
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Mon Oct 4 16:09:35 2021 +0300

    Merge branch 'master' into srp/urp-debugdisplay-api

    # Conflicts:
    #	com.unity.render-pipelines.universal/CHANGELOG.md
    #	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsCommon.cs
    #	com.unity.render-pipelines.universal/Runtime/Debug/DebugDisplaySettingsRendering.cs
    #	com.unity.render-pipelines.universal/Runtime/Debug/UniversalRenderPipelineDebugDisplaySettings.cs
    #	com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs

commit 945724e
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Thu Sep 16 14:26:26 2021 +0300

    Iterate m_Settings array instead of calling each settings object individually.

    - Changed DebugDisplaySettingsCommon interface implementation because of this. The common settings now report everything as "not active" because the widgets are not owned by it.

commit 17a8ef5
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Thu Sep 16 10:46:41 2021 +0300

    Fix casing to camelCase for settings properties

commit df48fa1
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Thu Sep 16 10:06:08 2021 +0300

    Changelog

commit 2971ab6
Author: Arttu Peltonen <arttu.peltonen@unity3d.com>
Date:   Thu Sep 16 09:15:36 2021 +0300

    Public API for URP DebugDisplaySettings

    - Made the settings API public
    - Renamed properties to be more uniform with each other and use camelCase
    - Added new Material Albedo validation preset: AlbedoDebugValidationPreset.Custom
@phi-lira
Copy link
Contributor

Merged onto staging branch. Staging branch landed onto master on #6085

@phi-lira phi-lira closed this Oct 22, 2021
@arttu-peltonen arttu-peltonen deleted the srp/urp-debugdisplay-api branch October 28, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants