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

[SG2]Imposter nodes and documents #7865

Open
wants to merge 21 commits into
base: sg2/main
Choose a base branch
from

Conversation

tracychen1234
Copy link
Contributor

@tracychen1234 tracychen1234 commented Apr 17, 2023


Purpose of this PR

Add imposter nodes and related documents to the node library.
image

Testing status

Connect the outputs of the nodes to the context node in an sg2 graph, and no error is shown.


Comments to reviewers

  • The nodes won't be working in sg2 because it needs vertex stage information. Feel free to test them in the unity repo pr with sg1 versions.
  • Find testing assets & sg1 version of the nodes on this slack thread

image

@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).
See the PR template for more information.
Thank you!

@tracychen1234 tracychen1234 requested a review from a team as a code owner April 17, 2023 21:45
This reverts commit dba3cd2.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better place to put this hlsl file?

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.

I need to do a bit more work on this to test our the actual nodes themselves but I won't be able to do that until tomorrow, so I'm just going to submit this bit now and then look at it some more tomorrow when I can switch to your branch and try it.

com.unity.sg2/Documentation~/previews/ImposterSample.md Outdated Show resolved Hide resolved
com.unity.sg2/Documentation~/Imposter-Sample-Node.md Outdated Show resolved Hide resolved

## Description

The Imposter UV Node calculates the billboard positon and the virtual UVs for sampling.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we say: "The Imposter UV Node calculates the billboard position (fix the typo in this word) and the UV coordinates needed by the Imposter Sample node." We need to indicate that data output here is intended specifically for the Imposter Sample node.

com.unity.sg2/Documentation~/Imposter-UV-Node.md Outdated Show resolved Hide resolved
com.unity.sg2/Documentation~/Imposter-UV-Node.md Outdated Show resolved Hide resolved
@"ImposterSample(HeightMapChannel, ViewDirectionTS, Parallax, Frames, Texture.tex, Texture.texelSize, Clip, Grid, UV0, UV1, UV2, Sampler.samplerstate, RGBA);",
new ParameterDescriptor[]
{
new ParameterDescriptor("Texture", TYPE.Texture2D, Usage.In),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an array texture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the current set up it needs to be a single texture split into frames.

com.unity.sg2/Documentation~/Imposter-Sample-Node.md Outdated Show resolved Hide resolved
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.

Making great progress on this!

@xiaoxicici xiaoxicici requested a review from esmelusina April 27, 2023 23:28
Copy link

@xiaoxicici xiaoxicici left a comment

Choose a reason for hiding this comment

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

I reviewed with Tracy live. Main discoveries are:

  • Align heightmap port naming with the existing Parallax node.
  • Texture size allows user input though would only work with square of 2s, something we might want to handle more elegantly, i.g. change the input field to dropdown selections. And user may have difference resolution on base color vs mask textures for example.

This is super valuable work and useful for users, so great job Tracy! LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants