PhysicsManager CHECK cleanup #305
Conversation
…Manager isntead of failing silently
facebook-github-bot
added
the
CLA Signed
| LOG(ERROR) << "Failed to remove object: no object with ID " << physObjectID; | ||
| return ID_UNDEFINED; | ||
| } | ||
| assertIDValidity(physObjectID); |
There was a problem hiding this comment.
Should the object destructor call removeObject()? That way you never forget to call it.
delete should be avoid and RTTI used instead. Could we use unique_ptr instead here? The last three lines would then boil down to:
existingObjects_.erase(physObjectID);
There was a problem hiding this comment.
This comes back to the SceneNode problem. SceneNode objects are owned completely by the SceneGraph. I have not gotten around to converting RigidObject from a SceneNode to an AbstractFeature3D as suggested in reviews for PR #91 . We use raw pointers to denote that PhysicsManager does not own the RigidObject.
That said, the removal process could probably be improved. I'll lump that in with the AbstractFeature3D re-design.
There was a problem hiding this comment.
If we don't own the object, why are we calling delete on it below on line 89?
There was a problem hiding this comment.
We have to remove the SceneNode from the SceneGraph when the object no longer exists. I agree the current situation is not entirely self-consistent. The simulator was not initially designed for this sort of dynamic restructuring.
…tation. (facebookresearch#305) * Integrated RGB noises * Integrated Depth noise * Added PyRobot noisy actuations action space * Integrated initial camera orientation * Updated objectnav_mp3d.yaml config * Improved overwrite_config to decrease code duplication * Added fix to respect CONTENT_SCENES for habitat baselines training
* replace if statements with CHECK for PhysicsManager and BulletPhysicsManager instead of failing silently
Motivation and Context
This PR replaces if statements in PhysicsManager and BulletPhysicsManager based on the validity of object IDs with CHECK().
As @msbaines mentioned in a review for PR #237, PhysicsManager functions taking an object ID as input should not fail silently when an invalid object ID is provided. This may allow hidden bugs to crop up. For example: setting a force or torque on an object which does not exist and then receiving no indication that the set failed resulting in extended debugging. Instead, such an invalid set should be fatal and produce a clear error message.
How Has This Been Tested
Local testing.
Types of changes
Checklist