Skip to content
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

Normalize view direction in universal #2752

Merged
merged 29 commits into from Jan 27, 2021

Conversation

elizabeth-legros
Copy link
Contributor

@elizabeth-legros elizabeth-legros commented Nov 25, 2020

Needed to have the view direction node return a normalized value in Shader Graph. Created templates in the Universal package to achieve this.

Ran all Shader Graph tests locally and verified normalized view direction in editor

@github-actions
Copy link

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

@elizabeth-legros elizabeth-legros requested a review from a team as a code owner November 25, 2020 19:51
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.

Missing changelog. Otherwise LGTM.

@GrantLamb-Unity GrantLamb-Unity requested a review from a user November 30, 2020 19:14
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I would like documentation updates both in the View Direction node documentation and the upgrade guide.

@@ -4,6 +4,8 @@

Provides access to the mesh vertex or fragment's **View Direction** vector. This is the vector from the vertex or fragment to the camera. The coordinate space of the output value can be selected with the **Space** dropdown parameter.

NOTE: In versions prior to 11.0, the **View Direction** vector was not normalized in the **Universal Render Pipeline**. Version 11.0 changed this behavior, and this vector is now normalized in both the **High-Definition Render Pipeline** and the **Universal Render Pipeline**. To mimic old behavior, you can use the [Position Node](Position-Node.md) in **World** space and subtract the **Position** output of the [Camera Node](Camera-Node.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if the camera position output in (local) World space in HDRP? (i.e. 0,0,0) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was something i wan going to have to double check in HDRP. I think you are right tho, and will address it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wording is getting changed tho, since weve added the new node

@phi-lira
Copy link
Contributor

@LandonTownsendUnity the feedback was addressed can you look at this PR again?

@elizabeth-legros
Copy link
Contributor Author

@LandonTownsendUnity the feedback was addressed can you look at this PR again?

We are also waiting on updated Documentation for the node before we can merge

@ghost ghost self-requested a review January 7, 2021 17:44
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I'm OK with the "view vector" object mode behavior being different because the old view direction behavior was normalized and that doesn't make sense for a "view vector" node. However it should be stated in the upgrade guide/documentation that you shouldn't swap out the node if it's in object space.

The behavior of "View Vector" is different from the old behavior of View Direction when in orthographic camera mode, in all spaces. This needs to be fixed. Even aside from upgrade problems, "view vector" being the vector from the position to the camera doesn't make sense in orthographic mode.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

View Vector needs to be fixed in HDRP - currently uses camera relative World transformations instead of Absolute World

Edit: actually hold on, let me check that

@ghost ghost self-requested a review January 12, 2021 18:30
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Validated both the length and direction in all possible combinations of
HDRP and URP
Orthographic and Perspective
Object, View, World, and Tangent space

My only worry is that someone will be relying on the behavior of the view direction being normalized in orthographic but not in perspective... however this seems like a very unlikely scenario.

Docs need the following changes:

  1. Documentation for the View Vector node needs to be added
  2. Documentation for the View Direction node needs to be updated to tell people to use the View Vector node
  3. Documentation for the View Direction node should specify that View Direction was in fact normalized in object mode prior to this fix, and that if a view direction node was in object mode, do not swap it out for the view vector node.

@ghost ghost self-requested a review January 14, 2021 20:19
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Changelog also needs to be updated with the fact that the View Vector node was added.

@ghost ghost self-requested a review January 14, 2021 20:20
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Now that the changelog is updated, I will approve.

@elizabeth-legros elizabeth-legros merged commit 446227e into master Jan 27, 2021
@elizabeth-legros elizabeth-legros deleted the sg/normalize-view-direction-shared-code branch January 27, 2021 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants