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

--Render skinning with PBR shader #2204

Merged
merged 19 commits into from
Nov 28, 2023
Merged

--Render skinning with PBR shader #2204

merged 19 commits into from
Nov 28, 2023

Conversation

jturner65
Copy link
Contributor

@jturner65 jturner65 commented Sep 14, 2023

Motivation and Context

This PR adds the capability of rendering skinned meshes using the PBR shader, making changes to the PBR vertex and fragment shaders to support the functionality, along with the loading pipeline. This PR is subsequent to this PR which promoted all the skin handling done by the drawables to the base Drawable class to make it available to both Generic(Phong/Flat) and PBR drawables.

Support of the various skin-related quantities used by the shader is being added in the PBRShader class, and the correct processing of these quantities by pbr.vert is also being added, using Magnum's Flat and Phong shaders' handling of skinning as a guide.

This PR also modifies viewer.cpp functionality to be more flexible and accept either urdf or ao_config.json files to load an Articulated Object.

Here's an example of some results on humanoid female_3.glb
pbrSkin

Since we're generating normals in the PBR shader already, no more tessellation artifacts and poly outlines.
pbrSkinCloseup
...compared to flat rendered
flatCloseup

Here's some interesting modifications I made in engine using some functionality I've been working on.
redAndTeal
porcelain

How Has This Been Tested

All c++ and python tests pass *
*(Had to modify existing test since we no longer force skins to be unlit. New test images are included in this PR)

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 14, 2023
@jturner65 jturner65 force-pushed the PBR_SkinRendering branch 7 times, most recently from 965fa47 to 00be243 Compare September 28, 2023 18:27
@jturner65 jturner65 force-pushed the PBR_SkinRendering branch 8 times, most recently from 8bc3ff1 to 5c0af5a Compare October 10, 2023 19:59
@jturner65 jturner65 force-pushed the PBR_SkinRendering branch 2 times, most recently from 96f23e6 to 21081c3 Compare October 16, 2023 17:43
@jturner65 jturner65 force-pushed the PBR_SkinRendering branch 2 times, most recently from ec12341 to 83c789a Compare October 26, 2023 15:25
@jturner65 jturner65 force-pushed the PBR_SkinRendering branch 2 times, most recently from c624ff9 to 17d2224 Compare November 17, 2023 01:02
@jturner65 jturner65 requested review from aclegg3, 0mdc and mosra and removed request for aclegg3 and 0mdc November 17, 2023 01:02
@jturner65 jturner65 requested a review from 0mdc November 17, 2023 01:03
@jturner65 jturner65 changed the title --[WIP]-Render skinning with PBR shader --Render skinning with PBR shader Nov 17, 2023
@jturner65 jturner65 marked this pull request as ready for review November 17, 2023 01:03
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.

Looks good! I left some minor comments.

I'll test it before approving.

{Mn::Trade::MaterialAttribute::Shininess, 80.0f}}};
return materialData;
} // ResourceManager::buildDefaultPhongMaterial

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

src/esp/gfx/PbrDrawable.cpp Show resolved Hide resolved
src/esp/gfx/PbrShader.cpp Show resolved Hide resolved
src/esp/gfx/PbrShader.cpp Show resolved Hide resolved
jointCount_, perVertexJointCount_, secondaryPerVertexJointCount_
#ifndef MAGNUM_TARGET_WEBGL
,
("mat4(1.0), "_s * jointCount_).exceptSuffix(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that I understand what's going on here. Could you add a comment above?

src/esp/gfx/PbrShader.cpp Show resolved Hide resolved
@@ -25,8 +25,8 @@ ArticulatedObjectAttributes::ArticulatedObjectAttributes(
// Set render mode to be default - skin if present, otherwise link
// meshes/primitives
setRenderMode(getAORenderModeName(ArticulatedObjectRenderMode::Default));
// Set default to be to use Phong shader
setShaderType(getShaderTypeName(ObjectInstanceShaderType::Phong));
// Set default to be to use the best shader for the material
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: to be to use

@jturner65 jturner65 merged commit cb12670 into main Nov 28, 2023
10 checks passed
@jturner65 jturner65 deleted the PBR_SkinRendering branch November 28, 2023 17:35
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