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

Order bounding box corners to be plottable #8

Merged
merged 2 commits into from
Jul 13, 2024

Conversation

GoldenZephyr
Copy link
Contributor

Is there a reason that the bounding box corners were not in order? It would be much more convenient for plotting or making polygon objects if the vertices were in this order. Are there any downstream ramifications of changing the order?

@GoldenZephyr
Copy link
Contributor Author

I can fix the unit test if we decide we want to make this change

Copy link
Collaborator

@nathanhhughes nathanhhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, afaik there is no downstream usage of this yet! The original ordering was meant to be "x,y,z" ordering (instead of the winding ordering that you have). Once the tests are fixed we can merge

@nathanhhughes
Copy link
Collaborator

Also: probably fine to just go into main if you'd like

@GoldenZephyr GoldenZephyr changed the base branch from develop to main July 13, 2024 15:47
@GoldenZephyr GoldenZephyr merged commit aa8c511 into main Jul 13, 2024
2 checks passed
@GoldenZephyr GoldenZephyr deleted the fix/plottable_bounding_boxes branch July 13, 2024 16:47
@Schmluk
Copy link
Contributor

Schmluk commented Jul 13, 2024

@GoldenZephyr Can you double check the hydra visualizer still works correctly? I'm not sure if I changed it to use the corners or only meant to do this. Also need to double check the khronos visualizers (maybe global search cornerns()?) Other than that no downstream uses that I know of.

@nathanhhughes
Copy link
Collaborator

My bad, had checked Hydra and Khronos (at least the private version) but forgot to actually search Hydra-ROS (will push a fix on the ros side)

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.

3 participants