--(Bugfix) Fix normal transformation calc and address backface culling in case of negative scaling/reflections #2062

jturner65 · 2023-04-05T22:27:18Z

Motivation and Context

It is common for an artist to use negative scaling to reflect an asset, using the same mesh for two different configurations. Habitat-sim was using the incorrect matrix to transform normals (the cofactor matrix vs. the inverse-transform of the rotationScale matrix), which became apparent when negative scaling was used in a SceneInstance configuration to instantiate a reflected asset in floorplanner dataset.

Reflection also changes winding order, which is used for backface culling. This PR addresses this as well, by changing backface culling winding order if reflection is caused due to a negative scale.

Below you can see this fix in action, the top image is what the scene looked like before the fix, and the bottom shows with the fix in place.

An Image comparison-based test was also added to SimTest to verify this process.

TODO (outside the scope of this PR):

Normal Matrix calc needs to be addressed in batch renderer (via Magnum change)
Backface culling needs to be addressed in batch renderer.

How Has This Been Tested

Existing tests passed C++ and Python

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 · 2023-04-05T23:25:10Z

src/esp/gfx_batch/Renderer.cpp

+                               .transformationMatrix.rotationScaling()
+                               .inverted()
+                               .transposed())


This addresses the erroneous normalMatrix() but does not yet address flipping the backFace culling winding value.

Fixing this for batch rendering is outside the scope of this PR.

src/esp/gfx/GenericDrawable.cpp

mosra · 2023-04-06T16:48:35Z

src/esp/gfx/GenericDrawable.cpp

@@ -143,7 +160,7 @@ void GenericDrawable::draw(const Mn::Matrix4& transformationMatrix,
              : node_.getSemanticId())
      .setTransformationMatrix(transformationMatrix)
      .setProjectionMatrix(camera.projectionMatrix())
-      .setNormalMatrix(transformationMatrix.normalMatrix());
+      .setNormalMatrix(invTransNormalMat);


Suggested change

.setNormalMatrix(invTransNormalMat);

.setNormalMatrix(normalMatrix);

That's what the matrix ultimately is, so I think it's clearest to name it like that. Same for the other drawable.

aclegg3

Solution looks good to me, thanks for fixing this!

I do wonder if there is any measure-able performance impact of additional maths in the draw() function. Can we check this before merging, maybe you and @mosra already thought about this.

Also, a nit: we're currently duplicating the solution and comments in PBRDrawable and GenericDrawable. Maybe this can be factored out into a function in base Drawable class for re-use?

jturner65 · 2023-04-06T20:38:23Z

I do wonder if there is any measure-able performance impact of additional maths in the draw() function. Can we check this before merging, maybe you and @mosra already thought about this.

Well, there really isn't anything additional than what was there. I'm just taking the calc apart so that I can query the determinant for face winding dir. (The inverse.transpose == cofactor/det )

Also, a nit: we're currently duplicating the solution and comments in PBRDrawable and GenericDrawable. Maybe this can be factored out into a function in base Drawable class for re-use?

I was going to do that but I needed two results - the boolean based on the determinant sign and the inverseTranspose normal matrix. I guess I could pass a ref and populate the results.

0mdc

LGTM!

It's probably impractical to do this here, but it would be helpful to provide a problematic asset so that we can quickly create a screenshot-based test. This would help addressing the batch rendering scenario.

jturner65 · 2023-04-07T12:44:42Z

LGTM!

It's probably impractical to do this here, but it would be helpful to provide a problematic asset so that we can quickly create a screenshot-based test. This would help addressing the batch rendering scenario.

Great Idea! I'll try to put that together and get it in by Monday.

src/esp/gfx_batch/Renderer.cpp

mosra · 2023-04-08T18:56:28Z

it would be helpful to provide a problematic asset so that we can quickly create a screenshot-based test This would help addressing the batch rendering scenario.

It'd be good to have for the classic renderer in any case. I imagine easiest would be to add a single sphere primitive to the scene, render it for a ground truth image, then scale it by -1 on X and render again -- it should give the exact same output.

However in the batch renderer it'll need a bit more depending on whether I'll go with a separate draw call or a specialized secondary index buffer. I'll be adding a test when I get to writing the full fix anyway, so no need to do it twice.

TODO : Still need to set this up in batch renderer.

The normal matrix derivation will be changed in magnum to be appropriate, so we will keep the original verbiage for clarity. We keep the changes in the default and PBR renderer calls since we need the determinant to determine if winding order must be reversed for backface culling.

The buffer is shared, so without a copy subsequent observations would overwrite the gt observation buffer.

jturner65 · 2023-04-11T19:56:46Z

The tests include image against file and image against image. This second test type required a copy of the observation buffer to be made for the ground truth, non-negative-scaled observation, since the observation buffer is shared (Otherwise subsequent observations would overwrite previous observations and they would always match).

* skip unsupported 3D primitives (#2054) * bugfix for setArticulatedObjectModelFilename causing garbage log output (#2053) * Gfx-replay polish: workaround for material-overrides; new keyframe getter (#2035) * gfx-replay polish: workaround for material-overrides; new keyframe getter API needed for a python application * --Have pre-commit use node version compatible with Ubuntu 18.04 (#2058) * --use system node install if present * --change to hook-specific node version specification * --change to nearest official release version * Fix viewer.py framebuffer size mismatch. (#2055) * Fix viewer.py framebuffer size mismatch. * Change int tuples to mn.Vector2. * Minor simplifications. * Replay renderer: add line-rendering and unproject() (#2057) * integrate DebugLineRender into replay renderers * add ClassicReplayRenderer.unproject * stub implementation for BatchReplayRenderer.unproject * lighting hack in ClassicReplayRenderer to get ReplicaCAD stages to render correctly * bugfix: cylinder proimitive collsion shape did not use halflength (#2060) * --have disabled renderer test only execute 1 time (#2065) * --(Bugfix) Fix normal transformation calc and address backface culling in case of negative scaling/reflections (#2062) * --don't use co-factor matrix for transforming pre-calculated normals * --address backface winding in Generic and Phong drawables TODO : Still need to set this up in batch renderer. * --revert change in batch renderer The normal matrix derivation will be changed in magnum to be appropriate, so we will keep the original verbiage for clarity. We keep the changes in the default and PBR renderer calls since we need the determinant to determine if winding order must be reversed for backface culling. * --test negative scaling along 3 axis * --make copy of observation buffer for ground truth observation The buffer is shared, so without a copy subsequent observations would overwrite the gt observation buffer. * Update Magnum submodules to latest. (#2066) * Update magnum submodules. (#2073) * Add runtime perf stats for troubleshooting perf problems (#2070) * add ResourceManager::getDrawableCountAndNumVertices, Sim.get_runtime_perf_stat_values, and related helpers * --Refactor Semantic Mesh loading/flattening to remove deprecated functionality and improve efficiency (#2079) * --refactor to remove deprecated functionality * --reviewer suggestions; * --fixed inappropriate alloc * Articulated object skinning (#2076) * Add rig property to RenderAssetInstanceCreationInfo. * Add render_asset to ao_config.json. * Create render asset from ao_config.json when creating an articulated object. * Store bone names into MeshTransformNode. * Add skinning asset loading and phong rendering. * Assorted minor fixes. * Add flag to render articulated object primitives while having a skinned mesh for debugging. * Add skinned articulated object test. * Fix MetadataMediatorTest * Cache joint transformations in drawables, other minor fixes. * Fix test asset path + other minor fixes. * Fix skinned mesh caching. Make ao_config.json render_asset path relative to the file. Code clean-up. * --Add access to Scene Instance-level user defined attributes. (#2081) * --add access to user defined in scene instance configs. * --If SceneDataset or SceneInstance do not exist, return nullptr. --------- Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com> * --Don't make needless copy of scene instance attributes when retrieving user defined values (#2082) * --don't make needless copy of scene instance attributes * --add a test * gfx-replay: fix to remove reflection when converting node transform matrix to rotation/translation (#2085) * Articulated object semantics (#2086) * Parse semantic_id from ao_config.json. * Propagate semantic_id to skinned mesh. * Add skinned mesh semantics test. * Propagate semantics for non-skinned articulated object. * --Convert materials to use magnum materials. (#2083) * --add Mn::MaterialTools; WIP * --address test issue; * --convert materials to magnum materials * --address fallback material not having defaults * --fix many attribute accessor bugs. * --appropriately make new attribute name with lowercase letter * --Flat/Phong and Semantic textures work. Have to fix building PBR layers * --improve ptr attribute naming; organize custom attribute assignment Still need to build texture pointer attribute array with layers if they exist in base material * --use owning material constructors * --cleanup; clang-tidy; fix flat vs phong ambient txtr map in phong shader Flat now has ambient Texture mapped directly from source material, so no need to check baseColorTexture anymore. * --get layers working properly * --support noneRoughnessMetallic texture; handle per-vert objectID for PBR drawables * --cache all material quantities in drawable to speed up draw access * --prepopulate normal texture * --address inappropriate bitflag check; add vertexID flag to PBR shader (#2090) * enable frustum-culling for classic replay renderer (#2096) * enable recompute_navmesh when creating sim with create_renderer==false (#2097) * --[BUGFIX] Reverse directional lights before sending to uniform; (#2094) * --reverse directional lights before sending to uniform; Also correct existing light setups * --fix lighting tutorial lights * --correct orientations of sample and default light with names * Update Magnum submodules. (#2100) * --[BUGFIX] Single channel texture support (#2102) * --Address uncompressed single and double channel textures by modifying swizzles to be rrr1 and rrrg respectively. * add intertiaFromURDF option to URDF loading API (#2098) * add additional magnum importer plugins to python build dependencies * refactor to reduce redundancy --------- Co-authored-by: Eric Undersander <eundersander@fb.com> Co-authored-by: John Turner <7strbass@gmail.com> Co-authored-by: Mikaël Dallaire Côté <110583667+0mdc@users.noreply.github.com> Co-authored-by: Alexander Clegg <alexanderwclegg@gmail.com> Co-authored-by: Vladimír Vondruš <mosra@centrum.cz>

jturner65 requested a review from mosra April 5, 2023 22:27

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 5, 2023

jturner65 marked this pull request as ready for review April 5, 2023 23:24

jturner65 requested a review from aclegg3 April 5, 2023 23:24

jturner65 commented Apr 5, 2023

View reviewed changes

jturner65 requested a review from 0mdc April 5, 2023 23:28

mosra reviewed Apr 6, 2023

View reviewed changes

src/esp/gfx/GenericDrawable.cpp Outdated Show resolved Hide resolved

src/esp/gfx/GenericDrawable.cpp Outdated Show resolved Hide resolved

src/esp/gfx/GenericDrawable.cpp Outdated Show resolved Hide resolved

mosra reviewed Apr 6, 2023

View reviewed changes

aclegg3 approved these changes Apr 6, 2023

View reviewed changes

0mdc approved these changes Apr 6, 2023

View reviewed changes

mosra reviewed Apr 8, 2023

View reviewed changes

src/esp/gfx_batch/Renderer.cpp Outdated Show resolved Hide resolved

mosra mentioned this pull request Apr 10, 2023

Update Magnum submodules to latest #2066

Merged

jturner65 added 6 commits April 10, 2023 16:45

--don't use co-factor matrix for transforming pre-calculated normals

51614cf

--address backface winding in Generic and Phong drawables

9a75ce8

TODO : Still need to set this up in batch renderer.

--reviewer comments

16e83cc

--naming suggestion

eb4f2a4

--test negative scaling along 3 axis

910d279

jturner65 force-pushed the fix-neg-scaling branch from d3de623 to 910d279 Compare April 10, 2023 20:47

jturner65 added 7 commits April 10, 2023 17:17

--modify placement

bc6b2d0

--improve test configuration

b335b65

--correct test config; add ground truth image

66693d4

--cleanup; add image-to-image comparison

606550a

--force test to fail on y-axis scale; comment out image2file test

72167a4

--remove forced failing; reinstate image-to-file test

d77c1d0

--make copy of observation buffer for ground truth observation

74ceb1b

The buffer is shared, so without a copy subsequent observations would overwrite the gt observation buffer.

jturner65 merged commit e446ab2 into main Apr 12, 2023

jturner65 deleted the fix-neg-scaling branch April 12, 2023 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--(Bugfix) Fix normal transformation calc and address backface culling in case of negative scaling/reflections #2062

--(Bugfix) Fix normal transformation calc and address backface culling in case of negative scaling/reflections #2062

jturner65 commented Apr 5, 2023 •

edited

Loading

jturner65 Apr 5, 2023

jturner65 Apr 6, 2023

mosra Apr 6, 2023

aclegg3 left a comment

jturner65 commented Apr 6, 2023

0mdc left a comment

jturner65 commented Apr 7, 2023

mosra commented Apr 8, 2023 •

edited

Loading

jturner65 commented Apr 11, 2023

	.setNormalMatrix(invTransNormalMat);
	.setNormalMatrix(normalMatrix);

--(Bugfix) Fix normal transformation calc and address backface culling in case of negative scaling/reflections #2062

--(Bugfix) Fix normal transformation calc and address backface culling in case of negative scaling/reflections #2062

Conversation

jturner65 commented Apr 5, 2023 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

jturner65 Apr 5, 2023

Choose a reason for hiding this comment

jturner65 Apr 6, 2023

Choose a reason for hiding this comment

mosra Apr 6, 2023

Choose a reason for hiding this comment

aclegg3 left a comment

Choose a reason for hiding this comment

jturner65 commented Apr 6, 2023

0mdc left a comment

Choose a reason for hiding this comment

jturner65 commented Apr 7, 2023

mosra commented Apr 8, 2023 • edited Loading

jturner65 commented Apr 11, 2023

jturner65 commented Apr 5, 2023 •

edited

Loading

mosra commented Apr 8, 2023 •

edited

Loading