[AO migration] ArticulatedObject API by aclegg3 · Pull Request #1312 · facebookresearch/habitat-sim

aclegg3 · 2021-06-14T15:36:51Z

Motivation and Context

This PR migrates base ArticulatedObject functionality from the prototype branch #1273.

Changes include:

ArticulatedObject base class with BulletArticulatedObject derived implementation.
URDFImporter base class and BulletURDFImporter derived implementation (construct BulletArticulatedObject from io::URDF::Model )
Basic API functionality: joint/DoF position|velocity|force/torque get/set, motion types, NavMesh support for Static, contact query, joint limits and clamping, link queries.
Wrapper/Manager interface for ArticulatedObjects.
URDF support in SceneInstance metadata

How Has This Been Tested

New tests for MetaDataMediator, AttributesManagers, and ArticulatedObject API.

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 · 2021-06-14T16:06:27Z

+template <>
+inline bool readMember(const JsonGenericValue& d,
+                       const char* tag,
+                       std::map<std::string, float>& val) {
+  if (d.HasMember(tag)) {
+    if (d[tag].IsObject()) {
+      const auto& jCell = d[tag];
+      for (rapidjson::Value::ConstMemberIterator it = jCell.MemberBegin();
+           it != jCell.MemberEnd(); ++it) {
+        const std::string key = it->name.GetString();
+        if (it->value.IsFloat()) {
+          val.emplace(key, it->value.GetFloat());
+        } else {
+          LOG(ERROR) << "Invalid float value specified in JSON config " << tag
+                     << " at " << key << ". Skipping.";
+        }
+      }  // for each value
+      return true;
+    } else {  // if member is object
+      LOG(ERROR) << "Invalid JSON Object value specified in JSON config at "
+                 << tag << "; Unable to populate std::map.";
+    }
+  }  // if has tag
+  return false;
+}  //  readMember<std::map<std::string, float>>


Something to note about this function - the 2 other container-based specializations of readMember will return true even if the value is not found in the json (they return false if some format error occurs while reading a tag), although this one does not (and it is expected that this one does not). The non-container versions return false if the tag is not present.

@eundersander and I have been discussing potentially changing this behavior for the container-based readMember, and their matching addMember methods. Just wanting to leave a note here about this.

aclegg3 · 2021-06-14T16:07:37Z


-    if (info.type == AssetType::FRL_PTEX_MESH) {
+    if (info.type == AssetType::PRIMITIVE) {
+      buildPrimitiveAssetData(info.filepath);


Note: this assumes the AssetAttributesManager has already registered a primitive asset with this name. As such, this won't support RenderReplay currently.

A possible solution to this would be the string-based construction of primitives. Since primitive attributes names encode their structure, and do not allow for renaming, we could reverse engineer the required attributes if it is missing by using the name.

I was thinking this might be a viable option also.

@eundersander FYI

Note that PR #1314 addresses this gap, thanks @jturner65!

jturner65

Once some method docstrings are provided, this looks great! Very exciting to get the main AO stuff into master.

Skylion007

A bunch of performance / style nits

bigbike

Really a big diff.
Not so sure if it can be further divided into even smaller ones.
Just reviewed part of it.

bigbike · 2021-06-14T17:42:58Z

+      std::shared_ptr<io::URDF::Material> material =
+          visual.m_geometry.m_localMaterial;
+      if (material) {
+        visualMeshInfo.overridePhongMaterial = assets::PhongMaterialColor();


are these models glbs? It is supposed to use Pbr instead of Phong. Where does it override such a choice (using Phong not Pbr)? Would you show me the a code pointer?

FYI: in Pbr, now I coded it up the shadows. So in the future, if we would like to leverage it, we need to use Pbr shader + Pbr drawables.

Good point. All of our test models define a 4d color. We've been treating them as Phong, but we could instead treat them as PBR with no textures.

Example of URDF defining this color.

Here is where this override material color is consumed to register a new Phong material and cache the custom LoadedAssetInfo.

bigbike · 2021-06-14T17:52:03Z

+        if (it->value.IsFloat()) {
+          val.emplace(key, it->value.GetFloat());
+        } else {
+          LOG(ERROR) << "Invalid float value specified in JSON config " << tag


It would be buried easily in the screen output.
would this error lead to fatal error in the future? If yes, use CORRADE_ASSERT here to stop it.

Counterpoint: LOG(ERROR) fits with our existing JSON-reading convention. An assert isn't appropriate because this is a runtime validation of data, whereas asserts are to catch programmer errors in code. Also, treating the error as fatal is inappropriate (it's up to the calling JSON code to decide the severity of a failed parse; some callers may want to just have the error logged but then keep going).

bigbike · 2021-06-14T17:57:18Z

-  explicit SceneObjectInstanceAttributes(const std::string& handle);
+  explicit SceneObjectInstanceAttributes(
+      const std::string& handle,
+      const std::string& type = "SceneObjectInstanceAttributes");


Just predict by the variable name: type.
I would define an enum for a type instead of using a string simply because a string can be random.
But I guess no chance to change it now (otherwise it requires a lot of code refactoring.)

Good point, agreed it seems like a later refactor.

…URDF parse/import errors.

eundersander

I left comments. Looks like just some straightforward code cleanup remaining.

eundersander · 2021-06-15T00:39:28Z

+template <>
+inline bool readMember(const JsonGenericValue& d,
+                       const char* tag,
+                       std::map<std::string, float>& val) {


This approach of implementing map<string, float> isn't going to scale very well to other types (you'll be copy-pasting a lot of code). For comparison, see our version of readMember for std::vector in this file.

Consider re-writing this to work for a map from string to a generic value type (templated on value type).

You can make this version more generic and still do error-handling here to detect and report a failed parse of a value.

Yep, I was planning on that refactor eventually, when time permitted.

eundersander · 2021-06-15T00:42:26Z

+        if (it->value.IsFloat()) {
+          val.emplace(key, it->value.GetFloat());
+        } else {
+          LOG(ERROR) << "Invalid float value specified in JSON config " << tag


Counterpoint: LOG(ERROR) fits with our existing JSON-reading convention. An assert isn't appropriate because this is a runtime validation of data, whereas asserts are to catch programmer errors in code. Also, treating the error as fatal is inappropriate (it's up to the calling JSON code to decide the severity of a failed parse; some callers may want to just have the error logged but then keep going).

eundersander · 2021-06-15T00:44:06Z

+ */
+
+template <>
+inline bool readMember(const JsonGenericValue& d,


How about a comment showing what the expected JSON looks like? This will help when someone needs to come along later and write an addMember (writer) for maps.

…stancing

eundersander

LGTM!

Alexander William Clegg added 6 commits June 11, 2021 18:33

URDF and Articulated Object Prototype and related demo assets

596fc2e

typo

75cedf8

Clean-up basic AO API

c868050

missing changes

2e96299

minor fixes

34e2696

Merge remote-tracking branch 'origin/master' into ao-migration-base-api

8a57c63

aclegg3 requested review from eundersander and jturner65 June 14, 2021 15:36

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jun 14, 2021

jturner65 reviewed Jun 14, 2021

View reviewed changes

Comment thread src/esp/metadata/MetadataMediator.h Outdated

jturner65 reviewed Jun 14, 2021

View reviewed changes

Comment thread src/esp/physics/bullet/objectWrappers/ManagedBulletArticulatedObject.h Outdated

jturner65 reviewed Jun 14, 2021

View reviewed changes

Comment thread src/esp/metadata/MetadataMediator.h

jturner65 reviewed Jun 14, 2021

View reviewed changes

aclegg3 commented Jun 14, 2021

View reviewed changes

jturner65 approved these changes Jun 14, 2021

View reviewed changes

aclegg3 requested a review from bigbike June 14, 2021 16:24

Alexander William Clegg added 3 commits June 14, 2021 10:25

doc and review comments

845b0fc

minor doc

9db6fba

include typo

c201305

Skylion007 reviewed Jun 14, 2021

View reviewed changes

Comment thread src/esp/physics/URDFImporter.cpp Outdated

Skylion007 reviewed Jun 14, 2021

View reviewed changes

Comment thread src/esp/physics/URDFImporter.h

Skylion007 approved these changes Jun 14, 2021

View reviewed changes

Comment thread src/esp/physics/bullet/BulletArticulatedObject.cpp Outdated

Comment thread src/esp/physics/bullet/BulletArticulatedObject.cpp Outdated

Comment thread src/esp/physics/bullet/BulletURDFImporter.h Outdated

bigbike reviewed Jun 14, 2021

View reviewed changes

Alexander William Clegg added 6 commits June 14, 2021 11:37

address review comments

855eeac

review code cleanup

cee167a

assert refactor

06801bd

refactor to assertion failure paradigm instead of silent failing for …

0089407

…URDF parse/import errors.

Merge remote-tracking branch 'origin/master' into ao-migration-base-api

42dd9ff

merge typo in test

e1883a5

eundersander reviewed Jun 15, 2021

View reviewed changes

Alexander William Clegg added 5 commits June 15, 2021 06:21

Merge remote-tracking branch 'origin/master' into ao-migration-base-api

0b11fd0

Move importURDFAssets into URDFImporter

c4c1d14

address reviews

25747f8

address reviews

7cd236d

Refactor Bullet URDF import pipeline.

4f1050f

Skylion007 reviewed Jun 15, 2021

View reviewed changes

Comment thread src/esp/physics/URDFImporter.cpp

Alexander William Clegg added 3 commits June 15, 2021 18:56

Refactored fixed base proxy feature, added test, added test asset

8494a95

Add light setup key to ArticulatedObject construction pipeline and in…

1d13671

…stancing

update test to reflect new number of test assets

6e9d296

aclegg3 requested a review from eundersander June 16, 2021 02:44

eundersander approved these changes Jun 16, 2021

View reviewed changes

aclegg3 merged commit b969422 into master Jun 16, 2021

aclegg3 deleted the ao-migration-base-api branch June 16, 2021 15:59

aclegg3 mentioned this pull request Jun 17, 2021

Is it possible to customize the texture of objects, or add new custom objects? facebookresearch/habitat-lab#473

Closed

Conversation

aclegg3 commented Jun 14, 2021

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jturner65 Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jturner65 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Skylion007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bigbike left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eundersander left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jturner65 Jun 14, 2021 •

edited

Loading

jturner65 left a comment •

edited

Loading