--[BE/Bugfix] Early Bullet/Physics enabled check; C++ test fixes for no-bullet; Python enable_physics default change if no_bullet #2189

jturner65 · 2023-08-18T15:56:46Z

Motivation and Context

This PR contains a bugfix and a few updates/improvements relating to whether Bullet dynamics library is present or not.

An ESP_CHECK was added early in Simulator::reconfigure() that would fail if the user has set their Simulator Configuration to have physics enabled but did not compile Habitat-Sim with Bullet enabled. If this is the state of things, we want it discovered as early as possible so that the user can address it before a long training regimen is initiated.
SimTest was updated to only set enablePhysics to true if built with Bullet, and also to test -all- tests using both SimConfig and MM-based simulator creation; furthermore, the tests that expressly rely on Bullet physics - createMagnumRenderingOff and testArticulatedObjectSkinned - are only enabled if Bullet is present and enabled.
PhysicsTest and ReplicaSceneTest also got similar treatment, where tests that were dependent on Bullet only were executed if built with Bullet.
The default python sim_cfg value for enable_physics was changed from True to "habitat_sim.bindings.built_with_bullet", which will still be true for folks who have bullet but will not automatically fail for folks who do not unless they explicitly change it.
Some python tests and examples were updated to only require Bullet/enable physics if they actually used dynamics, so that the test would still execute if Bullet was not present. Those examples that did require dynamics are now skipped properly if Bullet is not present.

How Has This Been Tested

Locally C++ and python tests pass. Some of the python tests required modifications or skipping if bullet was not present).

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.

aclegg3

A few minor nits ans suggestion.
Overall, looks good. I like your idea to fill the enable_physics default from the built_with_bullet arg. 👍

aclegg3 · 2023-08-21T20:58:15Z

src/esp/sim/Simulator.cpp

+            "cfg.enable_physics to False, or verify your Bullet installation "
+            "and recompile Habitat-Sim using the '--bullet' flag.");


Suggesting on the message advice. This change probably needs to be re-formatted with pre-commit.

Suggested change

"cfg.enable_physics to False, or verify your Bullet installation "

"and recompile Habitat-Sim using the '--bullet' flag.");

"cfg.enable_physics to False, or verify your Bullet installation (e.g. "

recompile Habitat-Sim using the '--bullet' flag or choose a 'withbullet' conda build).");

aclegg3 · 2023-08-21T21:05:37Z

tests/test_examples.py

@@ -63,10 +83,14 @@ def powerset(iterable):
        ("examples/tutorials/async_rendering.py",),
    ],
 )
-def test_example_modules(args):
+def test_example_modules_no_bullet(args):


Naming nit. Some of these (e.g. managed_rigid_object_tutorial) are optional physics rather than no-physics.

Suggested change

def test_example_modules_no_bullet(args):

def test_example_modules_optional_bullet(args):

aclegg3 · 2023-08-21T21:05:50Z

tests/test_examples.py

    run_main_subproc(args)


+@pytest.mark.skipif(
+    not habitat_sim.bindings.built_with_bullet,
+    reason="Bullet physics required for ReplicaCAD Artciulated Objects.",


Suggested change

reason="Bullet physics required for ReplicaCAD Artciulated Objects.",

reason="Bullet physics required for ReplicaCAD Articulated Objects.",

…ullet If bullet is not found or was not used to compile HabitatSim, enablePhysics should never be true. We want to fail early on this to make the user aware before training begins.

--Make enable physics true only if built with Bullet --Make all tests use both the SimConfig and MetadataMediator methods for creating a Simulator. TODO : disable tests that explicitly rely on Bullet if Bullet not found.

…th bullet. Note : there may now be situations where enable_physics is explicitly, although perhaps needlessly ,set to True that will now fail if no Bullet is present.

We just want to skip Bullet-dependent tests if Bullet is absent.

…ullet is present.

jturner65 requested review from aclegg3 and eundersander August 18, 2023 15:56

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

jturner65 changed the title ~~--[BE/Bugfix] Early proper Bullet/Physics enabled check and simTest fixes~~ --[BE/Bugfix] Early Bullet/Physics enabled check; C++ test fixes for no-bullet; Python enable_physics default change if no_bullet Aug 18, 2023

jturner65 force-pushed the BE_PhysicsCheckAndTestFixes branch 2 times, most recently from 2051c3a to 93edb9e Compare August 18, 2023 17:04

jturner65 mentioned this pull request Aug 21, 2023

--[BE/Bugfix]Modify creation of Articulated Object from URDF file name. #2190

Merged

11 tasks

jturner65 force-pushed the BE_PhysicsCheckAndTestFixes branch from 68d12c8 to 099a258 Compare August 21, 2023 21:17

aclegg3 approved these changes Aug 21, 2023

View reviewed changes

jturner65 added 7 commits August 21, 2023 18:39

--fail early in reconfigure if enablePhysics is set to true without b…

d635b7b

…ullet If bullet is not found or was not used to compile HabitatSim, enablePhysics should never be true. We want to fail early on this to make the user aware before training begins.

--Updates and fixes for SimTest

b81b35e

--Make enable physics true only if built with Bullet --Make all tests use both the SimConfig and MetadataMediator methods for creating a Simulator. TODO : disable tests that explicitly rely on Bullet if Bullet not found.

--address default settings.py setting enable_physics only if built wi…

d3e3117

…th bullet. Note : there may now be situations where enable_physics is explicitly, although perhaps needlessly ,set to True that will now fail if no Bullet is present.

--fix tests to not die if Bullet is not present

d38d6b6

We just want to skip Bullet-dependent tests if Bullet is absent.

--appropriately import built_with_bullet flag

bb676ed

--fix a few more Bullet installed-dependent tests.

3190845

--modify test_examples.py to only perform Bullet-dependent tests if b…

c644cb1

…ullet is present.

jturner65 force-pushed the BE_PhysicsCheckAndTestFixes branch from 099a258 to c644cb1 Compare August 21, 2023 22:40

jturner65 added 2 commits August 21, 2023 19:05

--reviewer suggestions

e3e2d25

--oops

0e81e02

jturner65 merged commit f65556a into main Aug 22, 2023
1 check passed

jturner65 deleted the BE_PhysicsCheckAndTestFixes branch August 22, 2023 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--[BE/Bugfix] Early Bullet/Physics enabled check; C++ test fixes for no-bullet; Python enable_physics default change if no_bullet #2189

--[BE/Bugfix] Early Bullet/Physics enabled check; C++ test fixes for no-bullet; Python enable_physics default change if no_bullet #2189

jturner65 commented Aug 18, 2023 •

edited

Loading

aclegg3 left a comment

aclegg3 Aug 21, 2023

aclegg3 Aug 21, 2023

aclegg3 Aug 21, 2023

		"cfg.enable_physics to False, or verify your Bullet installation "
		"and recompile Habitat-Sim using the '--bullet' flag.");

	def test_example_modules_no_bullet(args):
	def test_example_modules_optional_bullet(args):

	reason="Bullet physics required for ReplicaCAD Artciulated Objects.",
	reason="Bullet physics required for ReplicaCAD Articulated Objects.",

--[BE/Bugfix] Early Bullet/Physics enabled check; C++ test fixes for no-bullet; Python enable_physics default change if no_bullet #2189

--[BE/Bugfix] Early Bullet/Physics enabled check; C++ test fixes for no-bullet; Python enable_physics default change if no_bullet #2189

Conversation

jturner65 commented Aug 18, 2023 • edited Loading

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

aclegg3 left a comment

Choose a reason for hiding this comment

aclegg3 Aug 21, 2023

Choose a reason for hiding this comment

aclegg3 Aug 21, 2023

Choose a reason for hiding this comment

aclegg3 Aug 21, 2023

Choose a reason for hiding this comment

jturner65 commented Aug 18, 2023 •

edited

Loading