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

Allow cursors to be linked for plots #1722

Merged
merged 9 commits into from
Aug 28, 2022
Merged

Allow cursors to be linked for plots #1722

merged 9 commits into from
Aug 28, 2022

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jun 7, 2022

This adds set_link_cursor to LinkedAxisGroup which allow the cursor to show up in linked plots.

This only handles the default PlotItem case. I think changing PlotItem::on_hover to produce a list of cursors would make sense.

The PartialEq implementation for LinkedAxisGroup also seems a bit questionable given it carries identity.

@blitzarx1
Copy link

blitzarx1 commented Jun 22, 2022

That would be a nice contribution. Waiting for the feature! Thanks in advance ;)

@emilk
Copy link
Owner

emilk commented Jul 3, 2022

It would be nice of an example of this so we can test it!

@enomado
Copy link
Contributor

enomado commented Jul 27, 2022

Wanted this feature

@Zoxc Zoxc force-pushed the plot_cursor branch 2 times, most recently from 2ac8f80 to d826072 Compare July 29, 2022 17:30
@Zoxc Zoxc marked this pull request as ready for review July 29, 2022 17:30
@Zoxc
Copy link
Contributor Author

Zoxc commented Jul 29, 2022

I did some refactoring and added the ability to link cursors to the linked axis demo.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Very nice feature, but I think there is an opportunity to simplify the implementation while also making it more powerful!

egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved
pub(crate) bounds: Rc<Cell<Option<PlotBounds>>>,
cursors: Rc<RefCell<Vec<(Id, Vec<Cursor>)>>>,
Copy link
Owner

Choose a reason for hiding this comment

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

I agree with clippy

Copy link
Owner

Choose a reason for hiding this comment

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

Also needs documentation explaining it.

egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved
egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved
};

for pos in elem.arguments_with_ruler() {
push_argument_ruler(pos, shapes);
push_argument_ruler(pos, cursors);
Copy link
Owner

Choose a reason for hiding this comment

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

Not new in this PR, but I really don't see why push_argument_ruler exists and is not just inlined in the loop

@Zoxc Zoxc requested a review from emilk August 17, 2022 04:11
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Looks good, but the use of the word "frame" through me off. In egui it either means "one ui layout time slice" (as in "frames per second"), or it means egui::Frame (for drawing a border around widgets).

If think in this PR it just means "plot widget", so it would be better to call it CursorsForPlot or similar, with plot_id: Id for extra clairy.

Vertical { x: f64 },
}

/// Contains the cursors drawn for a specific frame of a plot.
Copy link
Owner

Choose a reason for hiding this comment

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

frame as in "frames per second"? Or just is it just "for a specific plot widget"? (each widget has a unique id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's what was drawn for a specific plot widget in a frame with the "frames per second" meaning.

.map(|(i, _)| i);

// Remove our previous frame and all older frames as these are no longer displayed.
index.map(|index| frames.drain(0..=index));
Copy link
Owner

Choose a reason for hiding this comment

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

I don't really follow this. What is frames sorted on? The docstring doesn't mention it being ordered by anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

frames is the oldest drawn widget to the newest. Each time a plot widget is drawn, it is appended to frames. Previous entries are removed here to avoid unbounded growth.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, now I see. Thanks for explaining! Could you please add those explanations to the code and then I will merge 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.

I added some more explanations.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Thanks!

@emilk emilk merged commit 9be060f into emilk:master Aug 28, 2022
@Zoxc Zoxc deleted the plot_cursor branch August 28, 2022 09:18
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