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

Make the space views bigger and less cluttered #269

Merged
merged 17 commits into from Nov 8, 2022

Conversation

emilk
Copy link
Member

@emilk emilk commented Nov 6, 2022

Part of PRO-186

I need to merge this first:

What?

This PR moves all of the 3D view's options, and most of the tensor view's options, to the selection panel.

Then, it puts a link to the space view options, together with a "?" help menu and an maximize-button (when needed) on top of the 3D and 2D space views (partially covering their content).

This is not possible for the tensor and text log views, so there some extra "headroom" is needed to fit the above buttons (still better than before though).

Before

Screen Shot 2022-11-08 at 01 01 03

After

Screen Shot 2022-11-08 at 01 00 30

More screenshots

Screen Shot 2022-11-06 at 21 01 08

Screen Shot 2022-11-06 at 21 10 52

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I've added a line to CHANGELOG.md (if this is a big enough change to warrant it)

@emilk emilk force-pushed the emilk/cleaner-space-view-ui branch 3 times, most recently from 83ed6ba to 98b4190 Compare November 7, 2022 11:56
@emilk emilk marked this pull request as ready for review November 7, 2022 12:43
@Wumpf
Copy link
Member

Wumpf commented Nov 8, 2022

something broke since you took the screenshots?
image
the fullscreen and settings buttons moved upwards for me, not rendered onto the viewports at they should

@Wumpf
Copy link
Member

Wumpf commented Nov 8, 2022

ah. The problem is that there is text AND 3d, it can't handle that correctly right now. Also it shouldn't show up for text views

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.

(not completely done yet. busy finishing up morning routine and jumping on bike)

crates/re_viewer/src/ui/view3d/mod.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/view3d/mod.rs Outdated Show resolved Hide resolved
Comment on lines +416 to +430
let frame = self.ctx.design_tokens.hovering_frame(ui.style());
hovering_panel(ui, frame, response.rect, |ui| {
if ui
.button("🗖")
.on_hover_text("Maximize Space View")
.clicked()
{
*self.maximized = Some(*space_view_id);
}

let space_view = self
.space_views
.get_mut(space_view_id)
.expect("Should have been populated beforehand");

space_view_ui(self.ctx, ui, self.spaces_info, space_view);
space_view_options_link(
self.ctx,
self.selection_panel_expanded,
*space_view_id,
ui,
"⛭",
);
Copy link
Member

Choose a reason for hiding this comment

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

only show for 3d and 2d space views? Placing isn't great right now when we have both text and 3d/2d subviews

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree placing isn't great, but I think users should still be able to maximize and go to settings for tensor views and text log views.

@emilk
Copy link
Member Author

emilk commented Nov 8, 2022

I introduced this bug in egui (weird extra distance between tooltips).

EDIT: found the bug: emilk/egui#2264

@emilk emilk force-pushed the emilk/cleaner-space-view-ui branch from 27d0600 to a082ef0 Compare November 8, 2022 10:38
@Wumpf
Copy link
Member

Wumpf commented Nov 8, 2022

Could we move down the controls into the same bar where we have the space view title for text and always go into the viewport if possible for "viewports"?
image

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.

code lgtm
But tbh I always have a hard time reviewing ui code changes that touch several places, hard to get a good overview. Want to experiment a bit more with the ui and think about the spacing issues already brought up

tensor: Tensor,
}

fn empty_tensor() -> Tensor {
Copy link
Member

Choose a reason for hiding this comment

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

why not as part of impl Tensor?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it makes some arbitrary decisions on dtype

@emilk
Copy link
Member Author

emilk commented Nov 8, 2022

I'll see if I can make the space view buttons for view_text and view_tensor better in a later PR.

@emilk emilk force-pushed the emilk/cleaner-space-view-ui branch from 49f1321 to 7791a80 Compare November 8, 2022 14:29
@emilk emilk merged commit 13dcb46 into main Nov 8, 2022
@emilk emilk deleted the emilk/cleaner-space-view-ui branch November 8, 2022 14:47
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.

None yet

2 participants