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

--[Bugfix/Refactor]-PBR Skin Rendering Prep/Bugfixes. #2222

Merged
merged 6 commits into from
Oct 4, 2023

Conversation

jturner65
Copy link
Contributor

@jturner65 jturner65 commented Sep 29, 2023

Motivation and Context

This PR addresses the following issues/refactors :

  • Some common Drawable functionality shared between GenericDrawables (Phong/Flat) and PBRDrawables was promoted to the base Drawable class. This includes management for lights, shader keys, and skin data and handling, in order to reduce needless code duplication and also as pre-requisite work for the upcoming PBR Skin rendering support.
  • The PBR shaders were originally implemented to perform calculations in model space, while the Phong and Flat shaders work in Camera space (ModelView transform). This caused the PBR shader to perform many redundant matrix multiplications unnecessarily. This distinction has been removed, and now PBR operates in Camera space, just like Phong and Flat shaders. This change is temporarily reverted until a reasonable solution to calculating IBL properly in camera space can be derived.
  • The PBR shaders were originally able to handle double-sided textures but this was disabled due to an issue with rendering artifacts being produced at edges. Since the PBR rewrite, this issue seems to have been addressed, and so this PR reactivates the ability to disable backface culling for double-sided textures.

How Has This Been Tested

All c++ and python tests pass locally

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 29, 2023
Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

LGTM! Left some very minor comments.

src/esp/gfx/Drawable.h Outdated Show resolved Hide resolved
src/esp/gfx/Drawable.h Outdated Show resolved Hide resolved
Corrade::Containers::arrayResize(jointTransformations_, jointCount);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

As discussed, there is a transform issue when projecting the environment map.

For reference, see the mirror in this video.

pbr_test-2023-10-02_11.50.20.mp4

@jturner65 jturner65 force-pushed the PBR_FixesSkinPrepRefactor branch 3 times, most recently from 01dc1a8 to 4dabed9 Compare October 2, 2023 17:16
@jturner65
Copy link
Contributor Author

As discussed, there is a transform issue when projecting the environment map.

For reference, see the mirror in this video.
pbr_test-2023-10-02_11.50.20.mp4

I'm reverting the changes to the PBR shader using camera space vs world space until this issue with IBl being calculated in camera space can be addressed properly.

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

LGTM! The issue is fixed with the latest iteration.

--promote shader key derivation to base Drawable
--move ownership of lightSetup and skinData to base Drawable.
The visual artifacts from turning off backface culling seem to have been addressed with the changes in the new PBR shader.
Removes unnecessary calculations separating and recombining Model and View matrices every draw.

NOT WORKING : IBL contributions need to be calculated in World space so that reflections are correct.
@jturner65 jturner65 merged commit bf8c6a3 into main Oct 4, 2023
1 check passed
@jturner65 jturner65 deleted the PBR_FixesSkinPrepRefactor branch October 4, 2023 09:58
@jturner65 jturner65 mentioned this pull request Oct 4, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants