Skip to content

[11.x] URP Shadergraph Fog Node should return fog intensity as density output#400

Closed
Looooong wants to merge 1 commit into
Unity-Technologies:masterfrom
Looooong:URP-shadergraph-fog-should-compute-fog-intensity
Closed

[11.x] URP Shadergraph Fog Node should return fog intensity as density output#400
Looooong wants to merge 1 commit into
Unity-Technologies:masterfrom
Looooong:URP-shadergraph-fog-should-compute-fog-intensity

Conversation

@Looooong
Copy link
Copy Markdown

Guide : https://github.com/Unity-Technologies/Graphics/blob/pr-template-correction/.github/pr-read.png.md

Display Addon : https://userstyles.org/styles/182991/unity-graphics-pr-readme

Purpose of this PR

Why is this PR needed, what hard problem is it solving/fixing?

In the URP, the Fog Node in Shadergraph outputs density using the result of ComputeFogFactor(z). However, the fog factor cannot be used mix fog color with fragment color. Instead, it must be passed to ComputeFogIntensity(fogFactor) to compute fog intensity, then this value is used to mix fog color with fragment color (this is how MixFogColor(fragColor, fogColor, fogFactor) is implemented). Therefore, instead of using fog factor, I suppose that Fog Node in Shadergraph should return fog intensity as density output. This will allow developer/designer to mix fog color using Lerp Node.

Testing status

Manual Tests

What have you tested?

Automated Tests

What did you setup? (Add a screenshot or the reference image of the test please)

Links

Yamato: (Select your branch) https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics

Any test projects or documents to go with this to help reviewers?

Comments to reviewers

Notes for the reviewers you have assigned.

@Looooong Looooong requested a review from a team as a code owner May 10, 2020 09:35
@Looooong Looooong requested a review from eh-unity May 10, 2020 09:35
@eh-unity
Copy link
Copy Markdown
Contributor

eh-unity commented Jun 1, 2020

Sorry for taking a long time to review.
I had to look into fog code and see how it was done for all the other shaders. I did compare the graph node to URP Unlit and the change appears to be correct. It makes the fog node in shadergraph more consistent with the URP shaders. Still not perfect in every case, but that might be a second bug somewhere else (possibly the Unlit shader).

However, it changes the existing look and also flips the correct input parameter order for the lerp mixing the fog with material color.
Please add a descriptive change log entry, so users know what has changed and how to fix, if this affects their projects.

@Looooong
Copy link
Copy Markdown
Author

Looooong commented Jun 3, 2020

I have added a changelog entry.

@phi-lira phi-lira requested a review from alindmanUnity July 16, 2020 10:49
@phi-lira
Copy link
Copy Markdown
Contributor

Adding @alindmanUnity as this is for shader graph.

@alindmanUnity
Copy link
Copy Markdown
Contributor

just an update, because this changes the output from previous behavior we need to have an upgrade path for this node, as well as upgrade documentation, before we can accept the change. Shader Graph team is stretched a little thin, so this might take a while to be addressed unless the fix is raised as high priority.

Copy link
Copy Markdown
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.

We need to add an upgrade path for this change in behavior before it can be merged.

@marctem marctem changed the title URP Shadergraph Fog Node should return fog intensity as density output [11.x] URP Shadergraph Fog Node should return fog intensity as density output Sep 29, 2020
@eh-unity eh-unity mentioned this pull request Nov 4, 2020
4 tasks
@phi-lira
Copy link
Copy Markdown
Contributor

Closing due to inactivity. If you are still working on it, please reopen PR again.

@phi-lira phi-lira closed this Dec 15, 2020
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.

5 participants