Skip to content

Set the USD schema multiccd scene flag default to True#3249

Open
LouRohanNV wants to merge 1 commit intogoogle-deepmind:mainfrom
LouRohanNV:fixUsdMultiCcdDefault
Open

Set the USD schema multiccd scene flag default to True#3249
LouRohanNV wants to merge 1 commit intogoogle-deepmind:mainfrom
LouRohanNV:fixUsdMultiCcdDefault

Conversation

@LouRohanNV
Copy link
Copy Markdown
Contributor

multiccd was enabled by default for 3.8.0, but the USD schema was not updated. Ideally there should be a test to detect this mismatch.

@LouRohanNV
Copy link
Copy Markdown
Contributor Author

I believe the proper test for this new change is to add these lines in mjc_physics_scene_test.cc around line 182:

  EXPECT_DISABLE_FLAG_USD_FALLBACK_EQ_MODEL_DEFAULT(IslandFlag, mjDSBL_ISLAND);
  EXPECT_DISABLE_FLAG_USD_FALLBACK_EQ_MODEL_DEFAULT(MultiCCDFlag,
                                                    mjDSBL_MULTICCD);

Also, I'm getting this from a review of the file:

The assertion line that was deleted in the "enable multiccd" commit was already broken on its own terms: the macro compared a bool to an int bitmask via EXPECT_EQ/EXPECT_NE, so 1 == (1<<19) is false for high bit positions. With mjENBL_MULTICCD = 1<<…, the assertion would never have actually matched the model default — it had to be replaced with the disable-flag form (mjDSBL_MULTICCD) anyway, but with the bool coercion fix it would have been a real check.

I'm not 100% certain if this is correct, but it suggests that these macros be adjusted to:

#define EXPECT_DISABLE_FLAG_USD_FALLBACK_EQ_MODEL_DEFAULT(usd_attr, mjc_flag) \
  {                                                                          \
    bool flag;                                                               \
    mjc_phys_scene.Get##usd_attr##Attr().Get(&flag);                         \
    EXPECT_NE(flag,                                                          \
              static_cast<bool>(default_model->opt.disableflags & (mjc_flag))); \
  }

#define EXPECT_ENABLE_FLAG_USD_FALLBACK_EQ_MODEL_DEFAULT(usd_attr, mjc_flag) \
  {                                                                          \
    bool flag;                                                               \
    mjc_phys_scene.Get##usd_attr##Attr().Get(&flag);                         \
    EXPECT_EQ(flag,                                                          \
              static_cast<bool>(default_model->opt.enableflags & (mjc_flag))); \
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant