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

Better log_pinhole and orient camera furstum based on pinhole view coordinates #2553

Closed
wants to merge 38 commits into from

Conversation

Wumpf
Copy link
Member

@Wumpf Wumpf commented Jun 28, 2023

What

Fixes #2244, Fixes #2589

Camera frustum and contained 2D content reacts now correctly to axis conventions set by view coordinates. Turns out that we before had essentially RDF hardcoded which became particularly apparent when flipping the Z axis, e.g. by using RUB (which ironically is the convention we use for the Eye camera itself).

I tested this against all samples that have a frustum and in particularly also made sure that 3D->2D transforms still work as expected (tested this in arkit and objectron).

Furthermore, I checked against #2512 where frustum orientation did not align with what happened when taking the view of the camera (i.e. double clicking the frustum). Without setting view coordinates this now correctly shows
image

And with setting

rr.log_view_coordinates("world/cameras", xyz="DLF") # TODO: Why is the camera wildly different?

the frustum flips around and the scene starts making sense
image

I also took the opportunity to look over uses of view coordinates and remove SpaceSpecs as well as unnecessary caching thereof (or view coordinates) as a result.

Checklist

PR Build Summary: https://build.rerun.io/pr/2553

Docs preview: https://rerun.io/preview/399c6d9/docs
Examples preview: https://rerun.io/preview/399c6d9/examples

@Wumpf Wumpf added the 🪳 bug Something isn't working label Jun 28, 2023
@Wumpf Wumpf marked this pull request as draft June 28, 2023 16:59
@Wumpf
Copy link
Member Author

Wumpf commented Jun 28, 2023

converted to draft since this doesn't account yet for projected rays

@Wumpf
Copy link
Member Author

Wumpf commented Jun 28, 2023

Phew. I think I understand situation now way better than before. Before the last couple of changes, the points we pass between 2D and 3D didn't work out because we had underlying assumptions of how axis translate. Now both the ray and the projected hover point work nicely:

hover point (just ehm imagine the mouse in the right spot)
image

hover ray:
image

@Wumpf Wumpf marked this pull request as ready for review June 28, 2023 20:04
jleibs
jleibs previously requested changes Jun 28, 2023
Copy link
Member

@jleibs jleibs left a comment

Choose a reason for hiding this comment

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

This PR seems to be conflating the role of view coordinates with the role of camera extrinsics. We do need to be able to use view coordinates to resolve the ambiguity in positive vs negative z. However, we should not arbitrarily be pointing the frustum along the axis that happens to have been annotated as F within a parent view coordinates. The only thing that should be changing which axis the camera is aligned with is a transform3d relative to the parent frame.

@Wumpf Wumpf force-pushed the andreas/fix-frustum-view-coordinates branch from 3e68f68 to 228dbbf Compare June 29, 2023 11:58
@emilk emilk marked this pull request as draft June 29, 2023 13:07
@emilk
Copy link
Member

emilk commented Jun 29, 2023

Converted to draft because we are not aligned with how to proceed on this yet. Basically we don't know exactly how view_coordinate should affect the frustum of a world/camera, if at all.

@Wumpf Wumpf force-pushed the andreas/fix-frustum-view-coordinates branch from ac0db9e to 399c6d9 Compare June 29, 2023 13:10
@emilk emilk changed the title Fix camera frustum not taking view coordinates into account Take view coordinates into account when orienting camera frustum Jul 3, 2023
@jleibs jleibs self-requested a review July 3, 2023 12:17
@jleibs jleibs dismissed their stale review July 3, 2023 12:19

Based on PR discussion. Unblocking this (but not approving yet).

@emilk
Copy link
Member

emilk commented Jul 3, 2023

I'm taking this over!

@emilk emilk changed the title Take view coordinates into account when orienting camera frustum Better log_pinhole and orient camera furstum based on pinhole view coordinates Jul 5, 2023
@emilk
Copy link
Member

emilk commented Jul 5, 2023

This has gone waaay above and beyond the original PR. I'll open my own PR instead :)

@emilk emilk closed this Jul 5, 2023
@Wumpf Wumpf deleted the andreas/fix-frustum-view-coordinates branch July 19, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify Pinhole without an explicit matrix ViewCoordinates are not working properly for camera frustums
3 participants