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

Determine bounding box coordinates ordering #60

Open
hdefazio opened this issue Jun 29, 2022 · 2 comments
Open

Determine bounding box coordinates ordering #60

hdefazio opened this issue Jun 29, 2022 · 2 comments

Comments

@hdefazio
Copy link
Contributor

No description provided.

@hdefazio
Copy link
Contributor Author

hdefazio commented Jun 29, 2022

from @Purg:

[A] We should add a comment about what the intended screen-space order of corners_screen_pos is, extending the comment on line 197.

[B] In terms of values being assigned to variables, it confused me that the "bottom" was being assigned to a "min" vertex, as "bottom" in screen space is the maximal value. It would have made more sense to me to leave the vertex variable assignment the same and change the assignment to corners_screen_pos instead.

[C] Not for this PR, but it seems we might want to change the names of the slots in the detection 3d set message as well. The cardinal direction naming presents a bundled assumption that, in screen-space (2D), the box is axis-aligned. That is not the case in the box resulting from the 3D transformation (not axis aligned in 3D), so those names should be expanded on in a revision.

@hdefazio hdefazio changed the title Determine naming convention for bounding box coordinates Determine bounding box coordinates ordering Jun 29, 2022
@Purg
Copy link
Member

Purg commented Jul 1, 2022

@joshanderson-kw's comment in that thread stated his originally intended ordering (BL, UL, UR, BR). Is there a technical issue with the current not-exactly-that ordering, or just the (lack of) clarity in the outgoing message attributes?

If we need to change the ordering, I presume that's a relatively easy change to the corners_screen_pos assignment.

I believe that either way we need to update the ObjectDetection3dSet message to use upper_left, lower_left, upper_right, lower_right naming to be more clear in 3D point meanings, adjusting message assignment as appropriate based on the outcome of the above query.

Ramifications of message change: users of that message need to update their code.

  • feedback generator
  • arui_engineering.unity related scripts that subscribed to object messages for rendering
    • Eventually to be replaced with CU's ARUI components, but I presume this would be a relatively quick fix.

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

No branches or pull requests

2 participants