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.

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

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

Merged
merged 6 commits into from
Oct 4, 2023
Merged

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

merged 6 commits into from
Oct 4, 2023

Conversation

jturner65
Copy link
Contributor

@jturner65 jturner65 commented Sep 29, 2023 •
edited
Loading

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.

@jturner65 jturner65 requested review from mosra, aclegg3 and 0mdc September 29, 2023 19:07
@jturner65 jturner65 force-pushed the PBR_FixesSkinPrepRefactor branch from 58cd8f3 to 85a95d2 Compare September 29, 2023 19:13
@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
0mdc
0mdc approved these changes Oct 2, 2023
View reviewed changes
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
src/esp/gfx/Drawable.cpp
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.

👍

0mdc
0mdc requested changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@0mdc 0mdc left a comment •
edited
Loading

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.

0mdc
0mdc approved these changes Oct 2, 2023
View reviewed changes
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.

@jturner65
--initial commit
2c4ab08 
--promote shader key derivation to base Drawable
--move ownership of lightSetup and skinData to base Drawable.
@jturner65
--promotion of shared skinning and lighting code to drawable base class
4fd7ec7
@jturner65
--re-activate disabling backface culling
f3cc192 
The visual artifacts from turning off backface culling seem to have been addressed with the changes in the new PBR shader.
@jturner65
--spelling;comment correction
56c5aea
@jturner65
--perform PBR calcs in ModelView (camera) space
38c999d 
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
Revert "--perform PBR calcs in ModelView (camera) space"
edf8038 
This reverts commit b1de9d2.
@jturner65 jturner65 force-pushed the PBR_FixesSkinPrepRefactor branch from c04537e to edf8038 Compare October 3, 2023 21:27
@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
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0mdc 0mdc 0mdc approved these changes

@mosra mosra Awaiting requested review from mosra

@aclegg3 aclegg3 Awaiting requested review from aclegg3

Assignees
No one assigned
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jturner65 @0mdc @facebook-github-bot