Skip to content
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

added ExtrinsicsToMatrix (CV) operation #57

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

mhusinsky
Copy link

can be useful when no conversion to Stride-space is wanted.

grafik

Name Change from ExtrinsicsToViewMatrix to ExtrinsicsToMatrix, because the extrinsics might also be used for object pose, not just camera pose.
@mhusinsky
Copy link
Author

mhusinsky commented Mar 15, 2022

I might have been a little quick with putting this here as PR, but would like to spark a discussion.

Simplification
I just updated this slightly as the patch could be simplified by removing a rotation which I believe is unnecessary (when applied, it needs to be undone like in the "Calculate a camera position using Aruco" help patch).

Before:
grafik
grafik
(Note that the two rotations ("Important" and "Rotate around world origin") are actually 2 sequential rotations around 180° and therefore cancel each other out).

After:
grafik

Name Change
The name ExtrinsicsToViewMatrix makes sense when applying the Matrix to the camera. When applying it to a scene object, it might be confusing. I therefore changed it to the more general ExtrinsicsToMatrix .
Another freedom I took was to create two versions of this node, one called ExtrinsicsToMatrix (CV) and one called ExtrinsicsToMatrix (CV to Stride). The latter one does perform a conversion to Stride coordinate convention (180° rotation around X-axis), while the first does not. This is because there might be cases where one wants to stay in CV space to perform multiple operations and only wants to move to Stride in the end.

Naming and usefulness is up for discussion...

grafik

@ravazquez
Copy link
Collaborator

Hello @mhusinsky,

Thanks for this!

I finally had the time to give it a closer look and it is all making sense so far, tests included.

My only two concerns are:

  1. Introducing a breaking change for this now seems a bit drastic. However, I believe that if we add the two nodes as you suggest, and keep ExtrinsicsToViewMatrix as a Legacy node to ensure people's old patches keep running, while also updating all help patches and docs to point at the new versions, we might be OK.
  2. The category naming does not feel quite right yet. Let me try and come up with an alternative.

Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants