Skip to content

Conversation

cdxntchou
Copy link

@cdxntchou cdxntchou commented Sep 14, 2021

Purpose of this PR

Fix for: https://fogbugz.unity3d.com/f/cases/1367540/

Backports:
2022.1: #5657
2021.2: #5801

Adding anisotropic option to the Sampler State node:
image

Docs ticket: https://jira.unity3d.com/browse/GSG-591


Testing status

Before: (no anisotropic)
image

After: (x16)
image

Added yamato test to test inline anisotropic sampler state node:
image

Yamato:

Nightly ShaderGraph on Trunk: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Fadd%252Fanisotropic/.yamato%252Fall-shadergraph.yml%2523Nightly_ShaderGraph_trunk/8832664/job/pipeline

  • Note that the OSX metal results on the anisotropic sampling test are incorrect, but it appears to just be from the integrated GPUs on the farm machines not supporting anisotropic. Running on a "real" OSX machine locally, the results look correct.

Console PR and Tests: 🟡 -- failures match master
https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/pull/244


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_trunk
Depending on your PR, you may also want
/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_trunk
/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_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.

Copy link
Contributor

@alindmanUnity alindmanUnity left a comment

Choose a reason for hiding this comment

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

Notes:

  • Move the aniso setting to the node inspector, so the setting is visible when the node is selected
  • Rename the setting to something easily searchable, ie "Anisotropy Filering", and the options to recognizable settings ie "x2, x4" etc
  • Cross check the setting name against any existing anisotropy settings (for example, on the texture assets) and when in doubt reuse the options for consistency.
  • Keep the default option consistent with the current node behavior.

@cdxntchou
Copy link
Author

Notes:

* Move the aniso setting to the node inspector, so the setting is visible when the node is selected

* Rename the setting to something easily searchable, ie "Anisotropy Filering", and the options to recognizable settings ie "x2, x4" etc

* Cross check the setting name against any existing anisotropy settings (for example, on the texture assets) and when in doubt reuse the options for consistency.

* Keep the default option consistent with the current node behavior.

Moved the control to the node inspector, renamed it to "Anisotropic Filtering", with options "None, x2, x4, x8, x16".
Default was already "None", including when upgrading from older graphs.

@alindmanUnity alindmanUnity self-requested a review September 21, 2021 16:48
Copy link
Contributor

@alindmanUnity alindmanUnity left a comment

Choose a reason for hiding this comment

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

approving with the UX changes

@cdxntchou cdxntchou marked this pull request as ready for review September 21, 2021 17:54
@cdxntchou cdxntchou requested a review from a team as a code owner September 21, 2021 17:54
@jessebarker
Copy link
Contributor

Quick comment on the test results (just to clarify the author's comment), the Intel integrated GPUs in our macOS test farm agents do not support this (and several other features, especially for HDRP), but an AMD or Apple Silicon GPU does support this (hopefully adding more agents in the future will help this).

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

@cdxntchou cdxntchou requested a review from a team September 21, 2021 21:25
@jessebarker jessebarker merged commit 4e40e2a into master Sep 21, 2021
@jessebarker jessebarker deleted the sg/add/anisotropic branch September 21, 2021 21:25
cdxntchou pushed a commit that referenced this pull request Sep 25, 2021
* Adding anisotropic option

* Shortening name

* Adding changelog

* Adding inline anisotropic test

* Updating Test Images

* Moved anisotropic control to node inspector, changed wording
# Conflicts:
#	com.unity.shadergraph/CHANGELOG.md
jessebarker pushed a commit that referenced this pull request Sep 29, 2021
* Adding anisotropic option

* Shortening name

* Adding changelog

* Adding inline anisotropic test

* Updating Test Images

* Moved anisotropic control to node inspector, changed wording
# Conflicts:
#	com.unity.shadergraph/CHANGELOG.md
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.

3 participants