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: Some objects have invalid scales when replay rendering #2200

Merged
merged 8 commits into from
Sep 27, 2023

Conversation

0mdc
Copy link
Contributor

@0mdc 0mdc commented Sep 8, 2023

Motivation and Context

When replay rendering (from the classic replay renderer, batch renderer, Unity or Blender), some objects have incorrect scaling.

In Habitat, the scale of nodes comes from two sources:

  • The CreationInfo scale, which is typically user-defined.
  • The asset scale. GLTF files (and other complex formats) are composed of nodes that can be scaled. Those are artist-defined.
    All importers will automatically apply the asset scale when loading a model.

To obtain the desired scale, we must multiply the CreationInfo scale with asset scale.

Because the recorder is writing the final scale (CreationInfo scale * asset scale) into keyframes, the renderers apply the asset scale twice when loading assets.

This changeset fixes replay renderer scaling by ensuring that only the CreationInfo scale is recorded, as the asset scale is already defined in the model.

Note: Upon importing URDF files, the graphics nodes were scaled directly rather than passing the information through CreationInfo. This would "hide" URDF scaling from gfx-replay.

The figure below shows results in Unity. Note that the behavior is the same in Blender, classic and batch replay renderers.

Before After
scale_before scale_after

How Has This Been Tested

Tested locally on classic and Unity replay renderers.

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 8, 2023
@0mdc 0mdc marked this pull request as ready for review September 10, 2023 22:30
@@ -296,14 +296,14 @@ bool BulletPhysicsManager::attachLinkGeometry(
Mn::Color4(material->m_matColor.m_specularColor);
}

auto scale = Mn::Vector3{1.0f, 1.0f, 1.0f};
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs to be scaled by the global scale specified in ao_config.json and accessed by artObjAttributes->getUniformScale() (see line 166 above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The global scale is already applied.

Model::setGlobalScaling in URDFParser.cpp sets m_meshScale, m_boxSize, m_sphereRadius, etc., which are applied below.

It's a bit confusing though, as GlobalScale/ing and UniformScale/ing names are used interchangeably but aren't directly applied.

@aclegg3
Copy link
Contributor

aclegg3 commented Sep 13, 2023

Marge main to pass CI 🙂

@aclegg3
Copy link
Contributor

aclegg3 commented Sep 13, 2023

Is there any testing for this?
I know there are replay tests, so maybe something that instances a scene and saves a replay, gets an image, loads the replay and compares a new images to the saved image?
Could catch many bugs with replay down the line.

Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

LGTM once tests pass :)

@0mdc 0mdc merged commit 8f5d58d into main Sep 27, 2023
1 check passed
@0mdc 0mdc deleted the replay-scaling-fix branch September 27, 2023 19:11
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.

5 participants