-
Notifications
You must be signed in to change notification settings - Fork 772
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
Update Hair Samples #6477
Update Hair Samples #6477
Conversation
This reverts commit af0da06.
kajiya still missing
# Conflicts: # com.unity.render-pipelines.high-definition/Samples~/MaterialSamples/Scenes/Hair.unity
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. HDRP 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. |
The whole samples weight ~11Mb (mesh, texture, shadergraph, materials), removing ~2Mb of the old hair samples, so this will increase hdrp package, will try to further slim file size |
…y-Technologies/Graphics into HDRP/Samples/hair_Marschner_samples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this!
One remark: can we enable the MSAA frame override for the game view camera? It will greatly reduce the aliasing for the strands. It would be even better to do it for the scene view too, but I am not sure if that is doable for samples.
The problem with MSAA is that we have to set up in the project settings, and the material samples have no power over that, it's just content, should I write a note about how to enable MSAA in the scene for better anti aliasing on the hair strands ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing status: Done
Very nice sample, visually pleasing and easy to use. Good default values, can quickly create hair variant of your own. Found a few minor issues
Whats tested
- Went over all files to check they are all used
- Took a look at all hair examples
- Tried to create hair of my own for each shader type (3 hair models), to verify every parameter works and its easy to use.
- Testing was performed on mac
Issues:
1) New warning
Just started testing. So far noticed a new warning "A meta data file (.meta) exists but its folder 'Packages/com.unity.render-pipelines.high-definition/Samples' can't be found"
Suggestion: Dont include file: "com.unity.render-pipelines.high-definition/Samples.meta" in the commit
2) Shaderpath (optional). We have a dedicated section for HDRP sample shadergraphs, but the hair ones are instead using generic location Shader Graphs
Suggestion: Edit this bit to move the shadergraphs to HDRPSamples
3) Hair naming inconsistencies. Most of the properties are using underscore naming An_Example, but two properties use camel case AnExample.
Suggestion: Simply use spaces if there is no reason not to. Or if there is an issue, then use camel case or underscores for everything
4) Empty notes
5) Text Its mostly okay, but needs a pass from doc team
...render-pipelines.high-definition/Samples~/MaterialSamples/Materials/HairCard_Dyed - Copy.mat
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Samples~/MaterialSamples/Scenes/Hair.unity
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Samples~/MaterialSamples/Scenes/Hair.unity
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Samples~/MaterialSamples/Scenes/Hair.unity
Outdated
Show resolved
Hide resolved
Added Subgraph, correct property naming, text, added the ability to set hair direction for all cases
I have the following suggestions to re-write the anotations in the attached images: A Physical hair shader that uses per-strand geometry. This example uses an opaque Material with no Textures. Instead, it uses vertex color for ambient occlusion, strand index, and radius. Each strand is a screen-aligned ribbon. For more details, see the PhysicalHair_Ribbon shadergraph. A Physical hair shader that uses transparent card geometry. This example uses the before refraction and alpha clip nodes. It writes to the motion vector to avoid jitters caused by Temporal anti-aliasing (TAA). It also uses transparent depth prepass with a threshold. For more details, see the PhysicalHair_Cards shadergraph. An Approximate Hair Shader that uses transparent card geometry. The Approximate model is based on the Kajiya-Kay algorithm. To use this model you have to manually balance the different components of the render, for example, basecolor, specular, and smoothness. For more details, see the ApproximateHair_Cards shadergraph. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added suggested changes to the images as a comment.
Screen Aligned Thickness and Depth Offset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All issues are fix from my side, can be merged when the documentation feedback is implemented
|
maybe a solution for sticky notes : edit text directly inside the shadergraph text file |
Found a workaround thanks to @SeanPuller by editing directly the shadergraph text file to make the sticky notes saved, just added some more precisions |
Purpose of this PR
New Hair scene for samples, showing different set up to render hair.
Testing status
Comments to reviewers
I need to do the Kajiya Kay (our old Hair shader) samples, that's why the materials on the hair strands are black.
Please check out the shadergraph and review them as well