From fc69ef1084936f8cd2b8c73031e98c15826069b3 Mon Sep 17 00:00:00 2001 From: Robin Alazard Date: Wed, 28 May 2025 13:19:06 -0700 Subject: [PATCH] Fix authoring type mismatch for geoms sizes attributes. Fixes wrong sizes being displayed in usdview. Also introduce a new test to ensure we catch as many of those kinds of errors as possible in the future. PiperOrigin-RevId: 764388588 Change-Id: Id3c878edd054fe5272c42c9f960c65c850655559 --- .../usd/plugins/mjcf/mujoco_to_usd.cc | 26 +++++----- .../usd/plugins/mjcf/mjcf_file_format_test.cc | 26 ++++++++++ test/experimental/usd/test_utils.cc | 48 +++++++++++++++++++ test/experimental/usd/test_utils.h | 4 ++ 4 files changed, 91 insertions(+), 13 deletions(-) diff --git a/src/experimental/usd/plugins/mjcf/mujoco_to_usd.cc b/src/experimental/usd/plugins/mjcf/mujoco_to_usd.cc index 69e03ebb..90dbf7e7 100644 --- a/src/experimental/usd/plugins/mjcf/mujoco_to_usd.cc +++ b/src/experimental/usd/plugins/mjcf/mujoco_to_usd.cc @@ -820,7 +820,7 @@ class ModelWriter { // MuJoCo uses half sizes. pxr::SdfPath size_attr_path = CreateAttributeSpec(data_, box_path, pxr::UsdGeomTokens->size, - pxr::SdfValueTypeNames->Float); + pxr::SdfValueTypeNames->Double); pxr::GfVec3f scale(static_cast(size[0]), static_cast(size[1]), static_cast(size[2])); SetAttributeDefault(data_, size_attr_path, 2.0); @@ -857,14 +857,14 @@ class ModelWriter { pxr::SdfPath radius_attr_path = CreateAttributeSpec(data_, capsule_path, pxr::UsdGeomTokens->radius, - pxr::SdfValueTypeNames->Float); - SetAttributeDefault(data_, radius_attr_path, size[0]); + pxr::SdfValueTypeNames->Double); + SetAttributeDefault(data_, radius_attr_path, (double)size[0]); pxr::SdfPath height_attr_path = CreateAttributeSpec(data_, capsule_path, pxr::UsdGeomTokens->height, - pxr::SdfValueTypeNames->Float); + pxr::SdfValueTypeNames->Double); // MuJoCo uses half sizes. - SetAttributeDefault(data_, height_attr_path, size[1] * 2); + SetAttributeDefault(data_, height_attr_path, (double)(size[1] * 2)); return capsule_path; } @@ -885,14 +885,14 @@ class ModelWriter { pxr::SdfPath radius_attr_path = CreateAttributeSpec(data_, cylinder_path, pxr::UsdGeomTokens->radius, - pxr::SdfValueTypeNames->Float); - SetAttributeDefault(data_, radius_attr_path, size[0]); + pxr::SdfValueTypeNames->Double); + SetAttributeDefault(data_, radius_attr_path, (double)size[0]); pxr::SdfPath height_attr_path = CreateAttributeSpec(data_, cylinder_path, pxr::UsdGeomTokens->height, - pxr::SdfValueTypeNames->Float); + pxr::SdfValueTypeNames->Double); // MuJoCo uses half sizes. - SetAttributeDefault(data_, height_attr_path, size[1] * 2); + SetAttributeDefault(data_, height_attr_path, (double)(size[1] * 2)); return cylinder_path; } @@ -917,8 +917,8 @@ class ModelWriter { pxr::SdfPath radius_attr_path = CreateAttributeSpec(data_, ellipsoid_path, pxr::UsdGeomTokens->radius, - pxr::SdfValueTypeNames->Float); - SetAttributeDefault(data_, radius_attr_path, 1.0f); + pxr::SdfValueTypeNames->Double); + SetAttributeDefault(data_, radius_attr_path, 1.0); WriteScaleXformOp(ellipsoid_path, scale); WriteXformOpOrder(ellipsoid_path, @@ -943,8 +943,8 @@ class ModelWriter { pxr::SdfPath radius_attr_path = CreateAttributeSpec(data_, sphere_path, pxr::UsdGeomTokens->radius, - pxr::SdfValueTypeNames->Float); - SetAttributeDefault(data_, radius_attr_path, size[0]); + pxr::SdfValueTypeNames->Double); + SetAttributeDefault(data_, radius_attr_path, (double)size[0]); return sphere_path; } diff --git a/test/experimental/usd/plugins/mjcf/mjcf_file_format_test.cc b/test/experimental/usd/plugins/mjcf/mjcf_file_format_test.cc index 4d35eb89..df41c739 100644 --- a/test/experimental/usd/plugins/mjcf/mjcf_file_format_test.cc +++ b/test/experimental/usd/plugins/mjcf/mjcf_file_format_test.cc @@ -37,9 +37,11 @@ #include #include #include +#include #include #include #include +#include // IWYU pragma: keep, used for TraverseAll #include #include #include @@ -450,6 +452,30 @@ TEST_F(MjcfSdfFileFormatPluginTest, TestKindAuthoring) { pxr::KindTokens->subcomponent); } +TEST_F(MjcfSdfFileFormatPluginTest, TestAttributesMatchSchemaTypes) { + // TODO(robinalazard): Make the scene much more comprehensive. We ideally want + // to test all the prims that the plugin can generate. + static constexpr char kXml[] = R"( + + + + + + + + + + + )"; + + pxr::SdfLayerRefPtr layer = LoadLayer(kXml); + auto stage = pxr::UsdStage::Open(layer); + + for (const auto& prim : stage->TraverseAll()) { + ExpectAllAuthoredAttributesMatchSchemaTypes(prim); + } +} + TEST_F(MjcfSdfFileFormatPluginTest, TestGeomsPrims) { static constexpr char kXml[] = R"( diff --git a/test/experimental/usd/test_utils.cc b/test/experimental/usd/test_utils.cc index 8b046793..aedb13b7 100644 --- a/test/experimental/usd/test_utils.cc +++ b/test/experimental/usd/test_utils.cc @@ -18,10 +18,13 @@ #include #include +#include #include +#include #include #include #include +#include #include #include #include @@ -71,5 +74,50 @@ void ExpectAttributeHasConnection(pxr::UsdStageRefPtr stage, const char* path, EXPECT_EQ(sources.size(), 1); EXPECT_EQ(sources[0], SdfPath(connection_path)); } + +void ExpectAllAuthoredAttributesMatchSchemaTypes(const pxr::UsdPrim& prim) { + // Get all properties on the prim that have authored opinions. + for (const pxr::UsdProperty& prop : prim.GetAuthoredProperties()) { + // We only care about attributes, as they are the ones with a typeName. + if (pxr::UsdAttribute attr = prop.As()) { + // 1. Get the official, composed schema type name for the attribute. + const pxr::TfToken schemaTypeName = attr.GetTypeName().GetAsToken(); + + // An empty schema type name means the attribute is not defined by + // a schema, or is of a dynamically-determined type. We can't + // check for a mismatch in this case. + if (schemaTypeName.IsEmpty()) { + continue; + } + + // 2. Get the property stack to check for authored opinions. + // The stack is ordered from strongest to weakest. + const pxr::SdfPropertySpecHandleVector propStack = + attr.GetPropertyStack(); + + for (const pxr::SdfPropertySpecHandle& spec : propStack) { + // We only care about attribute specs. + if (auto attrSpec = TfDynamic_cast(spec)) { + // 3. Check if this spec has an authored `typeName`. + if (attrSpec->HasField(pxr::SdfFieldKeys->TypeName)) { + const pxr::TfToken authoredTypeName = + attrSpec->GetTypeName().GetAsToken(); + + EXPECT_EQ(authoredTypeName, schemaTypeName) + << "Type mismatch for attribute <" << attr.GetPath() + << ">: expected schema-defined type '" + << schemaTypeName.GetString() << "', got authored type '" + << authoredTypeName.GetString() << "' in layer @" + << attrSpec->GetLayer()->GetIdentifier() << "@"; + + // We've found the strongest authored opinion for `typeName`, + // so we can stop checking the stack for this attribute. + break; + } + } + } + } + } +} } // namespace usd } // namespace mujoco diff --git a/test/experimental/usd/test_utils.h b/test/experimental/usd/test_utils.h index 655f06c9..77dd4ad9 100644 --- a/test/experimental/usd/test_utils.h +++ b/test/experimental/usd/test_utils.h @@ -103,6 +103,10 @@ void ExpectAttributeEqual(pxr::UsdStageRefPtr stage, void ExpectAttributeHasConnection(pxr::UsdStageRefPtr stage, const char* path, const char* connection_path); + +// Checks that all authored attributes on the given prim have types that match +// the schema types. +void ExpectAllAuthoredAttributesMatchSchemaTypes(const pxr::UsdPrim& prim); } // namespace usd } // namespace mujoco #endif // MUJOCO_TEST_EXPERIMENTAL_USD_PLUGINS_MJCF_FIXTURE_H_