Skip to content

Conversation

cdxntchou
Copy link

@cdxntchou cdxntchou commented Aug 31, 2021

Original PR here: #5452
Switched branches so I could rebase off of a passing master build.

Purpose of this PR

Fix for bug: https://fogbugz.unity3d.com/f/cases/1156544/

Console PR: https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/pull/234

Introduces a set of deterministic integer hashes, with varying input and output dimensions.
These hashes are integer based, so they produce the same result on ALL platforms.

Converted noise nodes to have enum switches to choose between legacy hash behavior, and the new deterministic hashes.
Existing noise nodes will use legacy behavior for backwards compatibility, newly created nodes choose deterministic by default.

New deterministic hashes also have a more random appearance: (legacy hash looks bad on nvidia cards)
image

Doc ticket added here: https://jira.unity3d.com/browse/GSG-548


Testing status

Added new test cases to the Procedural test scene, to test both legacy and deterministic versions of the noise nodes.
(noise node tests were previously disabled... enabled them)

Validated existing noise nodes upgrade with identical results, using the legacy hashes.
image

Tested newly created noise nodes, default to deterministic hashes:
image

Yamato:

ShaderGraph: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252Fnoise2/.yamato%252Fall-shadergraph.yml%2523Nightly_ShaderGraph_2021.2/8494066/job/pipeline
Master: 35e38b1
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-shadergraph.yml%2523Nightly_ShaderGraph_2021.2/8364525/job/pipeline

ShaderGraph Trunk: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252Fnoise2/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/8664161/job/pipeline
Master: 9f86ed3
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/8643605/job/pipeline

Console Tests: 🟡 - same failures as master
https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/pull/234


Comments to reviewers

Notes for the reviewers you have assigned.

@github-actions
Copy link

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)

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.

@cdxntchou cdxntchou changed the title Merge branch 'sg/fix/noise' into temp/merge [ShaderGraph] [2022.1] Making noise nodes platform deterministic Aug 31, 2021
@cdxntchou cdxntchou requested a review from bencloward August 31, 2021 17:14
@cdxntchou cdxntchou marked this pull request as ready for review August 31, 2021 17:30
@cdxntchou cdxntchou requested a review from a team as a code owner August 31, 2021 17:30
@cdxntchou cdxntchou requested a review from sarah-welton August 31, 2021 17:33
Copy link
Contributor

@bencloward bencloward left a comment

Choose a reason for hiding this comment

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

This is going to improve out noise a lot. Thanks, Chris!

@cdxntchou cdxntchou removed the request for review from sarah-welton August 31, 2021 22:38
sarah-welton added a commit that referenced this pull request Sep 1, 2021
Copy link
Contributor

@sarah-welton sarah-welton left a comment

Choose a reason for hiding this comment

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

Docs changes are in as requested. Seems like a great change, and happy to see it!

Copy link
Contributor

@jessebarker jessebarker left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sarah-welton sarah-welton left a comment

Choose a reason for hiding this comment

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

Forgot I already reviewed because I still had this open. Ignore me

Chris Tchou and others added 3 commits September 10, 2021 10:58
@jessebarker jessebarker merged commit 938b0f9 into master Sep 15, 2021
@jessebarker jessebarker deleted the sg/fix/noise2 branch September 15, 2021 21:41
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.

4 participants