Skip to content

Conversation

vincent-breysse
Copy link
Contributor

@vincent-breysse vincent-breysse commented Dec 1, 2021

/!\ Most of the changes are test related

Purpose of this PR

URP changes only

  • Merge FallbackError.shader and MaterialError.shader. These shaders were originally split to have a DOTS compatible error shader while still having a shader being compatible with built-in RP.
    However this PR tries to simplify this by having only one shader with two sub shaders and use the Tags { "RenderPipeline" = "UniversalPipeline" } thingy

URP/HDRP changes

  • Add #pragma editor_sync_compilation to error/loading shaders to avoid the "error/loading shader is currently loading" edge case
  • Add DOTS compatible loading shader
  • Some [Reload("Path/To/Shader.shader")] additions/updates

These new shaders are needed by Hybrid Renderer since built-in error/loading shaders aren't DOTS compatible.

Testing status

- Manual testing

- URP/HDRP QA pass

- Added URP/HDRP scenes to test various error shader cases when rendering with a BRG (not automated) :

- Compilation error in HLSL code
- Compilation error in shader parser (different internal handling than an error in HLSL code)
- Shader with missing DOTS_INSTANCING_ON keyword
- Material with missing shader
- MeshRenderer with missing material
- MeshRenderer with null material
- Shader non SRP Batcher compatible
- Shader not supported
- Shader pass not supported (different internal handling than whole shader not being supported)

- Yamato PR URP/HDRP on trunk green except:

Failing on master as well:
- URP_Lighting on Win_Vulkan_Standalone_mono_Linear on version trunk
- HDRP on Win_DX11_playmode_mono_Linear on version trunk
- HDRP on Win_DX11_playmode_XR_mono_Linear on version trunk
- HDRP on Win_DX12_playmode_mono_Linear on version trunk
- HDRP on Win_Vulkan_playmode_mono_Linear on version trunk
- HDRP on OSX_Metal_playmode_mono_Linear on version trunk
- HDRP on Linux_Vulkan_playmode_mono_Linear on version trunk
- HDRP_DXR on Win_DX12_playmode_mono_Linear on version trunk

@github-actions
Copy link

github-actions bot commented Dec 1, 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%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

URP
/jobDefinition/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

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.

@sebastienlagarde sebastienlagarde changed the title BatchRendererGroup error/loading material support [Draft] BatchRendererGroup error/loading material support Dec 14, 2021
@vincent-breysse vincent-breysse marked this pull request as ready for review January 10, 2022 18:40
@vincent-breysse vincent-breysse requested review from a team as code owners January 10, 2022 18:40
@vincent-breysse vincent-breysse changed the title [Draft] BatchRendererGroup error/loading material support DOTS compatible error/loading shaders Jan 10, 2022
Copy link
Contributor

@sebastienlagarde sebastienlagarde left a comment

Choose a reason for hiding this comment

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

small comment otherwise good for hdrp section

@vincent-breysse vincent-breysse force-pushed the graphics/brg-error-loading-shader branch from 4a64c56 to 68781bb Compare January 12, 2022 19:18
Copy link
Contributor

@TomasKiniulis TomasKiniulis left a comment

Choose a reason for hiding this comment

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

Tested BatchRendererGroup_HDRP, Amalienborg palace builds on Deferred and Forward with Game Objects and BRG. Looks good on HDRP side.

Test doc for more details: https://confluence.unity3d.com/pages/viewpage.action?pageId=172268208

Copy link
Contributor

@cinight cinight left a comment

Choose a reason for hiding this comment

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

# Conflicts:
#	com.unity.render-pipelines.high-definition/CHANGELOG.md
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/UniversalRendererData.cs
@vincent-breysse vincent-breysse changed the title DOTS compatible error/loading shaders DOTS compatible error/loading shaders GFXHYR-308 Feb 3, 2022
vincent-breysse and others added 4 commits February 3, 2022 14:33
# Conflicts:
#	com.unity.render-pipelines.high-definition/CHANGELOG.md
#	com.unity.render-pipelines.universal/CHANGELOG.md
# Conflicts:
#	TestProjects/BatchRendererGroup_HDRP/Assets/SampleScenes/BRGGameObjects/RenderBRG.cs
#	TestProjects/BatchRendererGroup_URP/Assets/SampleScenes/BRGGameObjects/RenderBRG.cs
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.

Co-authored-by: Felipe Lira <felipedrl@gmail.com>
@vincent-breysse vincent-breysse merged commit 80faba9 into master Feb 14, 2022
@vincent-breysse vincent-breysse deleted the graphics/brg-error-loading-shader branch February 14, 2022 18:15
@vincent-breysse vincent-breysse restored the graphics/brg-error-loading-shader branch March 4, 2022 12:39
@vincent-breysse vincent-breysse deleted the graphics/brg-error-loading-shader branch March 4, 2022 12:46
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.

6 participants