Skip to content

Conversation

JarkkoUnity
Copy link
Contributor

@JarkkoUnity JarkkoUnity commented Apr 15, 2021

Purpose of this PR

Enables motion vector rendering when using Graphics.RenderMeshInstanced()


Testing status

Tested motion vectors are properly rendered for RenderMesh() and RenderMeshInstanced() with a local HDRP project


Comments to reviewers

Requires landed C++ PR #126439 ("New draw API" for 2021.2)

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.

Please add a section in upgrade guide (the document name upgrade-guide-2021-2.md) to say the change done in the shader, thanks

@sebastienlagarde sebastienlagarde requested a review from a team May 28, 2021 18:15
@sebastienlagarde
Copy link
Contributor

Please add changelog + upgrade guide thanks

@sebastienlagarde
Copy link
Contributor

@iM0ve
Copy link
Contributor

iM0ve commented May 31, 2021

Also please update the PR description, remove all default text, briefly explain what the PR does and what testing took place. Thank you

@sebastienlagarde sebastienlagarde marked this pull request as ready for review July 7, 2021 15:25
@sebastienlagarde
Copy link
Contributor

Any update on this PR?

@github-actions
Copy link

github-actions bot commented Jul 7, 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://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

URP
/.yamato%252Fall-urp.yml%2523PR_URP_2021.2

Shader Graph
/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_2021.2
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_2021.2
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_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.

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.

So far running into an issue where universal package added to HDRP template introduces the following errors:
image (48)

@TomasKiniulis
Copy link
Contributor

The only issues that stands seems to be the Universal issue listed above. Tested RenderMesh() and RenderMeshInstanced() calls with all MotionVectorGeneration modes. HDRP/Lit, Fabric, LayeredLitTessellation, Unlit and basic SG Lit shaders, not other issues found.

@JarkkoUnity JarkkoUnity requested review from a team as code owners July 22, 2021 10:57
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.

Thanks for the fix! Switching pipelines to URP, HDRP and back to built-in works without any issues now.

@sebastienlagarde
Copy link
Contributor

sebastienlagarde commented Jul 28, 2021

Note for URP Qa: Change on urp side is just addition of those define
#define UNITY_PREV_MATRIX_M unity_MatrixPreviousM
#define UNITY_PREV_MATRIX_I_M unity_MatrixPreviousMI
otherwise code is share with HDRP in Core and instance file. + ABV is green (outside of iphone error coming from master)
HDRP QA coverage should be enough. It is not something SRP specific but SRP agnostic.

I will merge this PR to be part of SRP to core PR happening tomorrow. If you have any comment post merge, please put it here and we will adress them. Thanks

@sebastienlagarde sebastienlagarde merged commit 0305bdf into master Jul 28, 2021
@sebastienlagarde sebastienlagarde deleted the hd/new-draw-api branch July 28, 2021 12:15
@jessebarker
Copy link
Contributor

Note for URP Qa: Change on urp side is just addition of those define
#define UNITY_PREV_MATRIX_M unity_MatrixPreviousM
#define UNITY_PREV_MATRIX_I_M unity_MatrixPreviousMI
otherwise code is share with HDRP in Core and instance file. + ABV is green (outside of iphone error coming from master)
HDRP QA coverage should be enough. It is not something SRP specific but SRP agnostic.

I will merge this PR to be part of SRP to core PR happening tomorrow. If you have any comment post merge, please put it here and we will adress them. Thanks

This breaks BuiltIn. Were the tests run (they don't appear in the PR details and I can't find them on yamato)?

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.

7 participants