Updates to Teleport/TeleportFull and isStanding() #1241
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The purpose of these updates is to prevent an edge case where, when using an arm-typed agent, calling the Teleport action would cause the isStanding value to default in a weird way, causing the main camera of the arm-typed agent to be changed unexpectedly back to a default position that only has context for the high-level-agent. This was mainly due to the arm agents being derived classes of the high-level-agent's `PhysicsRemoteFPSAgentController` class - adding an override to the Teleport/TeleportFull actions in the `ArmAgentController` class so that an error can be thrown if passing in the `standing` bool since arm agents don't have a concept of a standing vs crouching camera position - updating the isStanding() helper function to throw an exception in cases where the High Level Agent's main camera is not in either the hard-coded standing or crouching local position. This has some issues as if UpdateMainCamera is called while using the HighLevelAgent, it causes an incompatible state
trying a fix where we allow returning a null value for `isStanding` for cases where the camera position has changed via UpdateMainCamera or for agents that don't have a meaningful use for isStanding like stretch agents
... that uses the high level "hand" rather than a physical arm also updating errorMessage for armed agents
AlvaroHG
reviewed
Oct 3, 2024
|
Minor comment, but LGTM! |
allowing epsilon to be parameterized, default to Unity's Vector3.kEpsilon, which apparently is built into the Vector3 class specifically because Unity's representation of floats is the 32-bit variety and that epsilon value is tuned for that??? So seems like a good default still
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose of these updates is to prevent an edge case where, when using an arm-typed agent, calling the Teleport action would cause the isStanding value to default in a weird way, causing the main camera of the arm-typed agent to be changed unexpectedly back to a default position that only has context for the high-level-agent. This update also catches a case where, when using the high-level agent and calling
UpdateMainCamera, the `isStanding() return became ambiguous.adding an override to the Teleport/TeleportFull actions in the
ArmAgentControllerclass and its derived classes so that an error can be thrown if passing in thestandingbool since arm agents don't have a concept of a standing vs crouching camera positionupdating the isStanding() helper function to return
nullin cases where the High Level Agent's main camera is not in either the hard-coded standing or crouching local position.