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

Adding DefaultCamera to Spatial3D view #8211

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jviereck
Copy link
Contributor

@jviereck jviereck commented Nov 22, 2024

At the moment it is not possible to set the camera position and view in the Spatial3D view. This PR implements this.

@jviereck
Copy link
Contributor Author

This PR needs to be rebased against the current main branch.

Please let me know what you mean about the overall shape of the changes. If they are good, I will rebase against main.

@Wumpf Wumpf self-requested a review November 25, 2024 09:11
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

at first I though it's an odd starting place to set the default camera given that the camera itself isn't specified yet
But I realize that this is the easiest entry point into being able to set the camera without having to worry about blueprint update frequency and the various other properties, so great initiative!

The implementation is off the mark however:

  • properties should be exposed in the ui, that isn't wired up here
  • there should be default provider implementations for these components now that do what default_eye did before.
    • default_eye then should always go to the components which may in turn be routed to the default providers
      -> this framework ensures that the assumed state of the property is transparent for the rest of the viewer (i.e. just the selection panel today)

I have a bit of time this week for smaller tasks like this, if you want I can take it from here


idea for a follow-up PR: extend the ui with a "set current" button that allows setting the default to the current camera pose. that way one can always return to it with double click on the viewer \o/

namespace rerun.blueprint.archetypes;


/// Defines a default camera view.
Copy link
Member

Choose a reason for hiding this comment

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

should explain more what this is: When is the default camera set, what happens when there is none.

Comment on lines +11 to +15
/// Origin of the camera view.
origin: rerun.blueprint.components.CameraOrigin ("attr.rerun.component_optional", nullable, order: 1100);

/// Target of the camera view.
target: rerun.blueprint.components.CameraTarget ("attr.rerun.component_optional", nullable, order: 1200);
Copy link
Member

Choose a reason for hiding this comment

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

should describe what happens if only one of those is set each

Comment on lines +6 to +7
"attr.docs.category": "Spatial 3D",
"attr.docs.view_types": "Spatial3DView, Spatial2DView: if logged above active projection",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"attr.docs.category": "Spatial 3D",
"attr.docs.view_types": "Spatial3DView, Spatial2DView: if logged above active projection",

We don't use those on property archetypes so far. (also the 2D space view part is obv. wrong)

@@ -14,4 +14,7 @@ table Spatial3DView (
/// If not specified, the default is to show the latest state of each component.
/// If a timeline is specified more than once, the first entry will be used.
time_ranges: rerun.blueprint.archetypes.VisibleTimeRanges (order: 10000);

/// Configures the default camera position of the 3D view.
Copy link
Member

Choose a reason for hiding this comment

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

similar as above, should clarify a little bit more what that means

@@ -92,6 +101,10 @@ fn ease_out(t: f32) -> f32 {
1. - (1. - t) * (1. - t)
}

fn vec3(v: Vec3D) -> Vec3 {
Copy link
Member

Choose a reason for hiding this comment

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

does into not work on those? thought that was implemented
Also, adding an unnamespaced vec3 function makes me a bit uneasy - glam and egui already have those as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pretty new to Rust, so not sure what you mean by into to be honest :/

@@ -41,6 +47,8 @@ use super::eye::{Eye, ViewEye};
pub struct View3DState {
pub view_eye: Option<ViewEye>,

pub view_eye_default: Option<ViewEye>,
Copy link
Member

Choose a reason for hiding this comment

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

why does the state need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See next comment.

Comment on lines +133 to +151
if let Ok(Some(pos_origin)) = default_camera.component_or_empty::<CameraOrigin>() {
if let Ok(Some(pos_target)) = default_camera.component_or_empty::<CameraTarget>() {
if self.view_eye_default.is_none() {
// Facking last eye interaction. Otherwise the next `if`
// statement in this function causes the camera to change
// if there is a change to the bounding box (e.g. when new
// objects are added to the scene.)
self.last_eye_interaction = Some(Instant::now());

self.view_eye_default = Some(from_to_eye(
vec3(pos_origin.0),
vec3(pos_target.0),
scene_view_coordinates,
));

self.view_eye = self.view_eye_default;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite follow why this is needed.
Shouldn't the behavior of default_eye be adjusted instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When double clicking on the 3D view, I want the camera to jump back to the default camera settings, if specified. The double click calls the View3DState::reset_camera method. While the default camera property is available from the gui-rendering path (default_camera.component_or_empty::<CameraOrigin>() etc), it is not so easy to query it from the reset_camera method. As such, I added a new field view_eye_default on the View3DState, which is set the first time the gui renders. This field is then accessible from the View3DState::reset_camera method.

I know this is not great and it would be cleaner to query to store for the CameraOrigin etc component directly, but I was not sure how to do that easily. So I decided to go for this workaround.



/// Defines a default camera view.
table DefaultCamera (
Copy link
Member

Choose a reason for hiding this comment

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

Maybe more like

Suggested change
table DefaultCamera (
table DefaultEyeCameraPose (

The problem with camera is that...

  • it gets confused with logged pinhole cameras a lot which is why we just call it eye in the implementation
    • eye alone though is probably even more confusing
  • this is not a full camera specification but just where it is. Later we'll have to add more things like camera controls (orbit vs ego vs future stuff) etc. which is then the "full specification" which will have a lot more fields

@jviereck
Copy link
Contributor Author

@Wumpf thank you for taking a look at this PR and your comments.

I agree with all your comments. I left a comment on why I've introduced the view_eye_default field. Hope this is clear.

If you manage to do so, I would appreciate if you can finish this up.

@Wumpf Wumpf self-assigned this Nov 27, 2024
@Wumpf
Copy link
Member

Wumpf commented Nov 27, 2024

putting this on draft for now until I resurrect it (hopefully later this week, maybe early next)

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.

Store (eye) camera properties of 3D View in Blueprint
2 participants