Fixing arm collision bug and cleaning up move/rotation actions.

[Lucaweihs]

This should only be merged after #801.

This PR:

• Fixes a bug where an objects held by the arm would not interact with other (non-kinematic) objects.
• Adds Move(Ahead/Back/Left/Right) and Rotate(Left/Right) actions to the arm controller.
• Removes the Move(Ahead/Back/Left/Right) and Rotate(Left/Right) actions form the base controller (these are always overwritten and so should not exist).
• Move the Move(Ahead/Back/Left/Right) and Rotate(Left/Right) to the action dispatch API.

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging a427abb into 35d7922 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

fixed alerts:

• 1 for Useless assignment to local variable

[mattdeitke]

Still looking over it, but as an initial review, there are now a few places where calling Move/Rotate will now break, if the user passes in certain kwargs.

Note: ArmAgent inherits from PhysicsRemoteAgentController, not Base!
https://github.com/allenai/ai2thor/blob/main/unity/Assets/Scripts/ArmAgentController.cs#L11

[mattdeitke]

Actually, on second thought.. I think a better solution would be to have a new agent controller for agentMode="default" that also inherits from PhysicsRemoteFPSAgentController, but does not include have any overlap with the agentMode="arm" controller (e.g., DefaultAgentController.cs)

This will make things much easier to maintain (e.g., if somebody adds a parameter to MoveAhead in PhysicsRemote, they won't also have to "catch" it in the AgentController.

[Lucaweihs]

Actually, on second thought.. I think a better solution would be to have a new agent controller for agentMode="default" that also inherits from PhysicsRemoteFPSAgentController, but does not include have any overlap with the agentMode="arm" controller (e.g., DefaultAgentController.cs)

@mattdeitke I started down this path but it quickly became a huge refactor that we should discuss with Eric (and everyone else) first. I completely agree that something like this should be done, especially as it's become really painful to figure our where certain actions are defined and there is a large amount of "action leakage" where certain actions are available to agents when they shouldn't be.

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging a578568 into edc4c37 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 7c35b47 into edc4c37 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging eb294ca into edc4c37 - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

fixed alerts:

• 1 for Useless assignment to local variable

[winthos]

looks good

[ehsanik]

Can this function result in a child having multiple parents that are disconnected? Basically, currently we will only have multiple parents for a child if there is a parent-child relationship between the two parents. Is this a correct observation?

[Lucaweihs]

The object hierarchy in Unity is a tree so every object can only ever have a single parent. All this function really does is ensure that two objects will move together.

[ehsanik]

I can confirm that these changes have fixed the bug Luca has mentioned. I tested it in this jupyter notebook: https://github.com/allenai/manipulathor/blob/bring_object/scripts/testing_lucas_changes.ipynb
Knocking kinematic objects in the room with the object in hand was not possible in the previous builds (it was only possible with the arm itself, which is why we missed this issue in my opinion). I verified that the bug is resolved on the notebook mentioned above.

[mattdeitke]

LGTM