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

Add support for editing a space view's space origin #4848

Merged
merged 21 commits into from
Jan 24, 2024

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Jan 17, 2024

What

This PR adds the possibility to edit a space view's space origin.

Note:This PR is affected by the following issue:

TODO:

  •  Are we happy with the general behaviour of this thing?
  • Should this be feature gated?
space_origin_edit.mp4

Follow-up issues:

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@nikolausWest
Copy link
Member

I expect to be able to arrow down to select one of the suggestions. Having to click on it feels broken.
Screenshot 2024-01-18 at 10 29 48

@nikolausWest
Copy link
Member

I find this interaction really surprising. If I click on an editable field I expect to be able to edit it, not that it will be a link.

We should remove the edit button and make it behave more like an editable field. The design should at the same time make it clear when other elements are in fact links and if that isn't clear then we need to address that instead

Screenshot 2024-01-18 at 10 31 41

@abey79 abey79 marked this pull request as draft January 18, 2024 10:51
@abey79 abey79 marked this pull request as ready for review January 18, 2024 15:50
@jleibs
Copy link
Member

jleibs commented Jan 22, 2024

I'm noticing a problematic interaction with heuristics we probably need to figure out.

In the SFM example, if I click the original space-view / and change it's origin to /camera, then it appears the heuristics re-create the / space-view again.

@nikolausWest
Copy link
Member

I'm noticing a problematic interaction with heuristics we probably need to figure out.

In the SFM example, if I click the original space-view / and change it's origin to /camera, then it appears the heuristics re-create the / space-view again.

We probably shouldn't be adding new views once the user has modified the blueprint. Once they have, we could consider highlighting the plus button and suggesting the top three views that would be added if we were to reset the blueprint

@abey79
Copy link
Member Author

abey79 commented Jan 23, 2024

I'm noticing a problematic interaction with heuristics we probably need to figure out.

In the SFM example, if I click the original space-view / and change it's origin to /camera, then it appears the heuristics re-create the / space-view again.

Is that blocking for this PR?

# Conflicts:
#	crates/re_viewer/src/ui/selection_panel.rs
@Wumpf Wumpf self-requested a review January 23, 2024 13:35
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.

the ui feels pretty nice, well done!

I left comments asking for major changes, but putting approve anyways since I'd be happy to move forward as is and taking those later

crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
crates/re_ui/src/lib.rs Outdated Show resolved Hide resolved
crates/re_ui/src/lib.rs Outdated Show resolved Hide resolved
crates/re_ui/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +159 to +166
if entered_editing {
output.response.request_focus();
let min = egui::text::CCursor::new(0);
let max = egui::text::CCursor::new(space_origin_string.len());
let new_range = egui::text::CCursorRange::two(min, max);
output.state.cursor.set_char_range(Some(new_range));
output.state.store(ui.ctx(), output.response.id);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@emilk I have an problem with this code. It should select all the text, but if the click happens on the text, then it selects only all the way to where the mouse cursor is. I can't make sense of that behaviour. Any idea what's going on?

Export-1706033974102

Copy link
Member

Choose a reason for hiding this comment

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

Weird - no I don't know

Copy link
Member Author

Choose a reason for hiding this comment

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

issue with repro here: emilk/egui#3883

@abey79 abey79 merged commit 756793e into main Jan 24, 2024
41 checks passed
@abey79 abey79 deleted the antoine/edit-space-origin branch January 24, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants