-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating Agent Metadata and ThirdPartyCamera Metadata #1239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good logically! Just a few tweaks for efficiency and typography's sake
cleaning up some comments and spelling, changing the following nomenclature: //for third party camera agentPositionRelativeThirdPartyCameraRotation -> agentRotationRelativeThirdPartyCameraRotation //for main camera agentPositionRelativeCameraRotation -> agentRotationRelativeCameraRotation as rotation relative to the agent position doesn't mean anything.
- removing extra inverse matrix operations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor naming change suggestion, otherwise LGTM.
unity/Assets/Scripts/AgentManager.cs
Outdated
cMetadata.parentPositionRelativeThirdPartyCameraPosition = | ||
camera.transform.localPosition; | ||
|
||
//get third party camera rotation as quaternion in parent space | ||
cMetadata.parentPositionRelativeThirdPartyCameraRotation = | ||
camera.transform.localEulerAngles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than parentPositionRelative...
can we change this to just parentRelative...
? I.e.
parentPositionRelativeThirdPartyCameraPosition -> parentRelativeThirdPartyCameraPosition
parentPositionRelativeThirdPartyCameraRotation -> parentRelativeThirdPartyCameraRotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! Just talked with rose about this to confirm that its still clear that the third party camera position is relative to the parent's position, and that the third party camera rotation is relative to the parent's rotation.
Note that there was a change previously to make parentPositionRelativeThirdPartyCameraRotation
to parent RotationRelativeThirdPartyCameraRotation
to make it explicit that the rotation can only be relative to rotation, and position can only be relative to position, but I think we can just do this in the description of the variables in the documentation rather than bloating the names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's not clear to me what it would mean for a rotation to be relative a position?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated with 12d8e05
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it's not clear to me what it would mean for a rotation to be relative a position?
Right, I think most people can intuit that position only makes sense relative to a position, and rotation to a rotation, so my original intent of being extremely explicit was probably overkill
shortening the following: - agentPositionRelativeThirdPartyCameraPosition -> agentRelativeThirdPartyCameraPosition - agentRotationRelativeThirdPartyCameraRotation -> agentRelativeThirdPartyCameraRotation - parentPositionRelativeThirdPartyCameraPosition -> parentRelativeThirdPartyCameraPosition - parentRotationRelativeThirdPartyCameraRotation -> parentRelativeThirdPartyCameraRotation
…world coords if null parent rather than returning null if no parent object, the parentRelativeThirdPartyCameraPosition and parentRelativeThirdPartyCameraRotation now default to world coordinates since za warudo is the parent now
This updates agent main camera and third party camera returned metadata to be more descriptive and have parity with each other in terms of naming convention and what values are returned. The idea is any camera, main or third party, will always return their position and rotation coordinates in world space and relative to the agent. Third party cameras additionally return position and rotation relative to any parent they might have as third party cameras may be attached to specific segments of articulated agents at some point.
This also fixes a silent bug where third party camera values may have not been correctly being calculated relative to the agent in some circumstances.
metadata now has
ThirdPartyCameraMetadata has been updated as well