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

Fix HDRP build issues with DOTS_INSTANCING_ON shader variant #6490

Merged
merged 3 commits into from
Dec 6, 2021

Conversation

vincent-breysse
Copy link
Contributor

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

Purpose of this PR

Fix shader compilation errors with DOTS_INSTANCING_ON variants. This issue has been introduced a few weeks ago with #5842. However, no one noticed it because of a C++ side bug causing some shader errors not to be logged anywhere. A trunk PR fixing this recently landed and caused these shader compilation errors to become visible (case 1367936). Also, the pathological shader variants were in fact not used at all, which is why we didn't notice any rendering or picking issue :D

Now regarding the problem itself:

  • The UnityPerDraw cbuffer doesn't exist with the DOTS_INSTANCING_ON shader variants, which means that unity_ObjectToWorld and friends don't exist as shader constants either. That kind of per-entity data needs to be loaded from the GPU buffer unity_DOTSInstanceData. This is done with a few helper functions that are called when using the UNITY_MATRIX_M macro. unity_ObjectToWorld should never be used directly as a shader constant with DOTS_INSTANCING_ON variants.

  • The macros used to fetch per-entity data like UNITY_MATRIX_M don't need, and should not be re-defined by the picking header when using the DOTS_INSTANCING_ON shader variants. It needs to be like this to match the way BatchRendererGroup objects picking is handled on the C++ side.


Testing status

  • Manual testing: Building/Running BatchRendererGroup_HDRP + Run BRGGameObjects in Editor

  • Yamato PR HDRP on trunk green except for the following:

    • Fail on master as well:
      HDRP on Win_DX11_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
      Find missing docs - hdrp
    • Instability on master, fail on this PR
      HDRP_DXR on Win_DX12_playmode_mono_Linear on version trunk
  • Yamato BatchRendererGroup on trunk green

@github-actions
Copy link

github-actions bot commented Dec 6, 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

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.

@vincent-breysse vincent-breysse changed the title FIX HDRP build issues with DOTS_INSTANCING_ON shader variant Fix HDRP build issues with DOTS_INSTANCING_ON shader variant Dec 6, 2021
@vincent-breysse vincent-breysse marked this pull request as ready for review December 6, 2021 16:13
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville left a comment

Choose a reason for hiding this comment

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

I don't know anything about dots, but it looks good HDRP wise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants