Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[HDRP][DXR] Remove Custom Interpolators from SpeedTree8 Graph to support raytracing #6370

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.

Sign up for GitHub

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

Jump to bottom
Merged
JulienIgnace-Unity merged 4 commits into from
Nov 25, 2021
Merged

[HDRP][DXR] Remove Custom Interpolators from SpeedTree8 Graph to support raytracing #6370

JulienIgnace-Unity merged 4 commits into from
Nov 25, 2021

Conversation

@remi-chapelain
Copy link
Contributor

@remi-chapelain remi-chapelain commented Nov 23, 2021 •
edited
Loading

Purpose of this PR

PR fixes the issue of black tree in the pathtracer. See GIF
b87541d8652dcc6055d3f38bd0b7c02e

The issue comes from the fact that the graph uses custom intepolators to pass data (normals, tangeants, bitangenats) through the vertex stage to the fragment stage. Custom intepolators are not supported in raytracing so it fails. Atfer talking with multiple people, no one saw the addition of using custom interpolators so they have been removed to support raytracing by default.

Here's the old and new graph.
Before
Before_Fix

After
image

Testing status

  • Added multiple trees in a scene and now they are rendering fine with the pathtracer. ✔️
  • Did the same with Ray Traced Reflection. ✔️

Comments to reviewers

  • They are still some more issue related to LOD with pathtracer with this graph but this is beyond the scope of this PR and it will be discussed after
  • It's not supposed to change anything on the rasterization side. (We have a test in the HDRP_Runtime test with tree (005) so issue will be caught there if anything. (when yamato will be back up)

@github-actions
Copy link

github-actions bot commented Nov 23, 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://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, 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
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_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.

@github-actions github-actions bot added the HDRP label Nov 23, 2021
@remi-chapelain remi-chapelain marked this pull request as ready for review November 23, 2021 16:06
@remi-chapelain remi-chapelain changed the title Add raytracing node to prevent custom intepolators [HDRP][DXR] Add raytracing node to prevent custom intepolators imcompatibilty with speed tree graph Nov 23, 2021
@remi-chapelain remi-chapelain changed the title [HDRP][DXR] Add raytracing node to prevent custom intepolators imcompatibilty with speed tree graph [HDRP][DXR] Remove Custom Interpolators from SpeedTree8 Graph to support raytracing Nov 23, 2021
TomasKiniulis
TomasKiniulis approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@TomasKiniulis TomasKiniulis left a comment

Choose a reason for hiding this comment

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

Thanks for the change! I tried the fix applied in Xpipeline Village and it worked nicely - as mentioned in the PR, had no additional issues on rasterization side and trees were properly shaded in pathtracing.

@JulienIgnace-Unity JulienIgnace-Unity merged commit ba56bc1 into master Nov 25, 2021
@JulienIgnace-Unity JulienIgnace-Unity deleted the HDRP/fix-speedtree-for-raytracing branch November 25, 2021 13:27
@sebastienlagarde
Copy link
Contributor

sebastienlagarde commented Nov 25, 2021

@remi-chapelain reverted the PR as something bad have happen with various error message and something about exceeding variant limit. I will redo one

@sebastienlagarde
Copy link
Contributor

sebastienlagarde commented Nov 25, 2021

#6397

@sebastienlagarde sebastienlagarde mentioned this pull request Nov 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sebastienlagarde sebastienlagarde Awaiting requested review from sebastienlagarde

2 more reviewers

@eturquin eturquin eturquin approved these changes

@TomasKiniulis TomasKiniulis TomasKiniulis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dxr HDRP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@remi-chapelain @sebastienlagarde @eturquin @TomasKiniulis @JulienIgnace-Unity
Morty Proxy This is a proxified and sanitized view of the page, visit original site.