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

Slightly nicer tensor view #1028

Merged
merged 15 commits into from
Feb 2, 2023
Merged

Slightly nicer tensor view #1028

merged 15 commits into from
Feb 2, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Feb 1, 2023

  • shorter selector name label
  • only show axes on hover
  • longer slider

tensor-ui-improvements

Moving the sliders below the tensor image is difficult in immediate mode. We could move them to the very bottom of the space view area, but I think that would be too far.

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)

@jleibs
Copy link
Member

jleibs commented Feb 1, 2023

What's the reasoning behind "only show axes on hover?" Like everything this should probably be configurable via blueprint in the long-run.

Maybe I'm wrong, but my intuition is that on average more users would want this to be visible all the time instead of needing to mouse-over the image to find out how things are oriented.

@emilk
Copy link
Member Author

emilk commented Feb 1, 2023

@nikolausWest has the motivation. I think it is "less visual clutter". It's also not something you need to be constantly reminded of. Once you've learned it, you know it.

@jleibs
Copy link
Member

jleibs commented Feb 1, 2023

Once you've learned it, you know it.

I don't think that's true. Especially when you start to set up multiple different tensor views with different slice permutations.

Needing to mouse over every window and memorize the configuration and hold that in your head while also trying to make sense of the complexity of tensor data projections feels like a bad user experience to me.

I know it's a fine line but I think this is a case of letting design/style get in the way of practical day-to-day utility.

@nikolausWest
Copy link
Member

I don't think that's true. Especially when you start to set up multiple different tensor views with different slice permutations.

Needing to mouse over every window and memorize the configuration and hold that in your head while also trying to make sense of the complexity of tensor data projections feels like a bad user experience to me.

I know it's a fine line but I think this is a case of letting design/style get in the way of practical day-to-day utility.

I think longer term it might be nice to have this showing all the time for those that want it. In practice, I've not yet found I have the need to see the axes all the time. It's not so much about design as that these small elements add up, making it so that a large proportion of the view is just padding when you have lots of tensors. Also, I do find that I generally go through a period of inspecting everything to make sure it's correctly set up, followed by just wanting to look at the actual data. When I'm inspecting I absolutely want to know the axes' orientation but after that I don't want to think about it all the time.

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.

lgtm.. except those arrows, am I the only one? 😄

crates/re_viewer/src/ui/view_tensor/ui.rs Outdated Show resolved Hide resolved
let text_rect = if invert_height {
// On top, pointing up:
let galley =
painter.layout_no_wrap(format!("➡ {height_name}"), font_id, text_color);
Copy link
Member

Choose a reason for hiding this comment

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

to me these arrows look flipped, should be

Suggested change
painter.layout_no_wrap(format!(" {height_name}"), font_id, text_color);
painter.layout_no_wrap(format!(" {height_name}"), font_id, text_color);

(and same below)

Copy link
Member Author

Choose a reason for hiding this comment

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

The text is at the high end of the scale, and the arrows point in the direction of increase, like most graphs:

image

What I found confusing is that the X-label is not always printed at Y=0, and the other way around.

In any case, nothing new in this PR.

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 fixed the cross-align of them now, and added some ascii art

crates/re_viewer/src/ui/view_tensor/ui.rs Outdated Show resolved Hide resolved
@nikolausWest
Copy link
Member

The dimension indicators seem to overlay on the slider. Can we rearrange stuff to avoid it?

@jleibs
Copy link
Member

jleibs commented Feb 1, 2023

Can we rearrange stuff to avoid it?

If they didn't need to overlay something we could just leave them visible all the time. I think the choices for overlay are either the tensor itself (which will raise questions of contrast) or the slider.

@emilk
Copy link
Member Author

emilk commented Feb 1, 2023

The dimension indicators seem to overlay on the slider. Can we rearrange stuff to avoid it?

We can add a margin, like we had before (but maybe only at the top).

Or we can have it overlay the image.

emilk and others added 3 commits February 1, 2023 21:09
Co-authored-by: Andreas Reich <andreas@rerun.io>
@emilk
Copy link
Member Author

emilk commented Feb 1, 2023

I added some margin at the top:

Screen Shot 2023-02-01 at 21 32 52

@emilk
Copy link
Member Author

emilk commented Feb 1, 2023

I improved the cross-positioning of the labels:

Screen Shot 2023-02-01 at 21 45 41

Screen Shot 2023-02-01 at 21 45 55

@emilk emilk merged commit c3151dd into main Feb 2, 2023
@emilk emilk deleted the emilk/nice-tensor-view branch February 2, 2023 09:08
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.

4 participants