Skip to content

Conversation

adrien-de-tocqueville
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Jul 7, 2021

Purpose of this PR

Added a ShaderGraph node to get the sun light direction for URP and HDRP.


Testing status

Tested the node in a shadergraph with a HDRP and URP target.
Tested on the preview material.
Verified the node preview output.
Tested with no directional light (gives 0 vector)

Verified the node follows these guidelines: https://confluence.unity3d.com/pages/viewpage.action?spaceKey=SHAD&title=Node+Style+Guide

Example in HDRP:
image

@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 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.

@github-actions github-actions bot added the HDRP label Jul 7, 2021
@github-actions
Copy link

github-actions bot commented Jul 7, 2021

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page).
See the PR template for more information.
Thank you!

@adrien-de-tocqueville adrien-de-tocqueville requested review from a team as code owners July 7, 2021 15:32
@sebastienlagarde sebastienlagarde changed the title Added an SG node and API to get the Sun light direction Added an SG node and API to get the Sun light direction [22.1] Jul 12, 2021
@adrien-de-tocqueville adrien-de-tocqueville changed the title Added an SG node and API to get the Sun light direction [22.1] Added an SG node to get the Main light in HDRP and URP [22.1] Jul 13, 2021
Copy link

Choose a reason for hiding this comment

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

Working well on URP lit/unlit. This will be a great addition for unlit!

Copy link
Contributor

@phi-lira phi-lira left a comment

Choose a reason for hiding this comment

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

LGTM. I think this solves some issues we have as well with users trying to do this with custom node.

I wonder about shadows. We see users do custom node for main light shadows as well. In URP this would be easy to support either as separate node or part of same node.

@phi-lira phi-lira requested a review from Verasl September 6, 2021 13:28
@adrien-de-tocqueville
Copy link
Contributor Author

I wonder about shadows. We see users do custom node for main light shadows as well. In URP this would be easy to support either as separate node or part of same node.

Sadly this is not as easy for HDRP, so this should probably be URP only

Copy link
Contributor

@alex-vazquez alex-vazquez left a comment

Choose a reason for hiding this comment

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

Looks good to me, tested on a XPipeline Project

Copy link
Contributor

@Verasl Verasl left a comment

Choose a reason for hiding this comment

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

Not sure that there was a thorough planning for this feature, in URP this is a pointless node as a user would still have to make a custom main light node to get the colour and shadows. This means a broken incomplete workflow which I'm not sure we want to go down.
If HDRP cannot provide this information then I ask why bother with this node if it only supports 1/3 for the data a user would want in URP and not a great experience on HDRP side?

Furthermore with the coming Forward+ in URP does it make sense to have this separation in shader graph, as the concept of a main light is a URP only thing and something made as a fast path for simple Forward games that only required a single directional light?

@adrien-de-tocqueville
Copy link
Contributor Author

@Verasl There are certainly uses cases that would need colour and shadow information for URP, but these wouldn't be cross pipeline workflows.
On the other hand there are use cases that only require the direction of the light, so this node allows them to be done in a cross pipeline way.

Regarding the main light concept, it's something we have in HDRP regardless of the shading mode. And for URP the node is a wrapper around GetMainLight already available in shaders so if there is a problem it should be handled there

@sebastienlagarde
Copy link
Contributor

@Verasl There are certainly uses cases that would need colour and shadow information for URP, but these wouldn't be cross pipeline workflows.
On the other hand there are use cases that only require the direction of the light, so this node allows them to be done in a cross pipeline way.

Regarding the main light concept, it's something we have in HDRP regardless of the shading mode. And for URP the node is a wrapper around GetMainLight already available in shaders so if there is a problem it should be handled there

As a note: we have indeed get request for a cross pipeline way of getting main light direction.
Users use it to do effect for the sun light, like darkening grass based on sun orientation or doing self shadow for POM for sun. In automotive they also have other use case.

Indeed for URP itself users will prefer to access other light properties but for HDRP due to pre-exposure concept that can quicky create a wrong visual this is not recommended.

@sebastienlagarde sebastienlagarde requested a review from a team September 21, 2021 10:40
@sebastienlagarde sebastienlagarde removed the request for review from a team September 24, 2021 09:18
@sebastienlagarde sebastienlagarde merged commit 0c59248 into master Sep 24, 2021
@sebastienlagarde sebastienlagarde deleted the hd/sg-sun-light-node branch September 24, 2021 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants