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

[hd] Properly set up both the shadow projection and view matrices #583

Conversation

pmolodo
Copy link
Contributor

@pmolodo pmolodo commented Aug 6, 2018

Description of Change(s)

For some fancy shadowing updates we're working on, we need access to both the view + projection parts of the shadow / light projection. The data structures are actually set up to support this, but at the point where they're populated, it essentially just sets the projection, and sets the view matrix to identity. This makes the HdxSimpleLightTask properly set the light's view / transform matrix as well.

Of course, in order for this to work properly, the SimpleLight has to be properly set up, so that IT transform + shadow matrices are set up properly / separately. Currently, nothing in the main code base actually sets the the shadow matrix - but it's possible you folks might have third party code which is, and it may need to be adapted. ie, if you are passing in a shadow matrix which is actually the combined view + transform matrix, AND setting the light transform to non-identity, this will need to be corrected.

Fixes Issue(s)

  • None (but provides new capabilities for shadows)

@jtran56
Copy link

jtran56 commented Aug 23, 2018

Filed as internal issue #164140.

@chadrik
Copy link
Contributor

chadrik commented Sep 10, 2018

Hi all, we'd love to get a set of eyes on this. It's a simple change, so hopefully if there are no philosophical disagreements we can get it merged quickly! We'll follow up in the next few weeks with our fancy shadow PR.

@poljere
Copy link
Contributor

poljere commented Sep 11, 2018

Hi! The reason for the View matrix to be identity in Hdx/SimpleLightTask is because (as you said) internally we are actually combining view/projection into the shadow matrix.

We could change this behavior internally, but we would like to understand how this will be used first. Maybe with the followup change it will be more clear?

Thanks!

@pmolodo
Copy link
Contributor Author

pmolodo commented Sep 11, 2018

If I recall correctly, the main thing that I needed the light-space xform for was to compare distances - both "perpendicular" distances within viewing planes, and depth distances. I'm implementing percentage-closer soft shadows (PCSS - http://developer.download.nvidia.com/shaderlibrary/docs/shadow_PCSS.pdf) - which makes use of several "similar triangles" comparisons to calculate values, thus the need to have these distances in the same space. After the shadow projection, it's easy enough to extract the depth from the w component, but the relation between this and the "perpendicular" distances obviously becomes distorted.

Also, when GlfSimpleShadowArray was designed, it was clearly intended to be used with separate light xform + shadow matrix components: the setter methods are called "SetViewMatrix" and "SetProjectionMatrix" and the "GetWorldToShadowMatrix" method then explicitly multiplies them together. At best, it's confusing to have an API where you're required to SetViewMatrix to identity - if we truly want to always collapse the two, then the API should be renamed / reworked to reflect that, and only take a single worldToShadowProjection matrix, not two separate matrices. However, I think that the designer correctly anticipated that having two separate matrices would be useful, and we should keep the current design.

@poljere
Copy link
Contributor

poljere commented Sep 19, 2018

Thanks for the PR and for the great explanation. We have patched this internally and it should be available in our next release.

@superfunc
Copy link
Contributor

@poljere Do you mean it will be available in the next dev branch push?

@pixar-oss pixar-oss merged commit c3aefb1 into PixarAnimationStudios:dev Sep 25, 2018
pixar-oss added a commit that referenced this pull request Sep 25, 2018
[hd] Properly set up both the shadow projection and view matrices

(Internal change: 1893932)
@pmolodo pmolodo deleted the pr/separate_shadow_view_projection branch March 12, 2019 23:03
AdamFelt pushed a commit to autodesk-forks/USD that referenced this pull request Apr 16, 2024
…lve/adsk/feature/ImportedTargets

Find idiff From OpenImageIO::idiff Target
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.

7 participants