-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Draw axis labels and ticks outside of plotting window #2284
Conversation
Support for labels is done. I changed the API for egui_labels.webm |
I am testing out this branch. Would you like feedback or wait until you finish developement? |
I would love some feedback if you have time. However, somebody will have to review this again in 1-2 weeks because I plan to make the axis-API a bit more convenient. |
❯ ./sh/check.sh
+ cargo install cargo-cranky
Updating crates.io index
Ignored package `cargo-cranky v0.3.0` is already installed, use --force to override
+ export 'RUSTFLAGS=--cfg=web_sys_unstable_apis -D warnings'
+ RUSTFLAGS='--cfg=web_sys_unstable_apis -D warnings'
+ export 'RUSTDOCFLAGS=-D warnings'
+ RUSTDOCFLAGS='-D warnings'
+ cargo check --all-targets
Finished dev [optimized + debuginfo] target(s) in 0.30s
+ cargo check --all-targets --all-features
Checking serde v1.0.143
Checking tracing v0.1.36
Checking zerocopy v0.6.1
Compiling speech-dispatcher-sys v0.7.0
Checking wgpu-hal v0.14.0
Compiling zvariant_derive v3.6.0
Compiling zbus_macros v2.3.2
Checking resvg v0.23.0
Checking smithay-clipboard v0.6.6
Checking web-sys v0.3.60
Checking sctk-adwaita v0.4.2
Checking winit v0.27.2
Checking half v1.8.2
error: failed to run custom build command for `speech-dispatcher-sys v0.7.0`
Caused by:
process didn't exit successfully: `egui/target/debug/build/speech-dispatcher-sys-511605648ffbec9f/build-script-build` (exit status: 101)
--- stdout
cargo:rustc-link-lib=speechd
--- stderr
wrapper.h:1:10: fatal error: 'speech-dispatcher/libspeechd.h' file not found
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ClangDiagnostic("wrapper.h:1:10: fatal error: 'speech-dispatcher/libspeechd.h' file not found\n")', $CARGO_HOME/registry/src/github.com-1ecc6299db9ec823/speech-dispatcher-sys-0.7.0/build.rs:22:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish... Everything else seems ok. |
Thanks for working on this, and it looks fantastic! I do like this feature, but one immediate problem I see is how much whitespace it uses up, especially on the X axis. Take a look at these before-and-after pictures from the demo: That's basically half as much plot as before. Perhaps we need to tweak the default. I see no need for the names here ("x" and "y") when they were not explicitly given. And the left side could probably have a thinner default width. Is it configurable? |
Thanks for your feedback.
Can be changed in the
The width is configurable by I didn't figure out how to determine the size of one digit without drawing it first...
In issue #889 I discussed this and made a proposal for automatically calculated y-axis-width. I am not sure it'd pay off because this feature might introduce some weird edge case behavior and makes the widget layout computation unnecessarily complex.
I think plots without axis labels should be the exception, not the other way round. If someone really wants a plot without axis labels it should be specified explicitly. But if you are sure about removing labels by default I'll just do it. |
I think this is a user problem. That is: users should name their axes, and we should encourage that (e.g. by always doing so in all examples), but "x" and "y" aren't names. If the user is plotting cpu usage over time, calling the axes "x" and "y" is probably worse than having no names at all. |
I agree with @emilk. Axis labels should be optional, but should be promoted in all examples. |
6fd5b9d
to
677f896
Compare
This reverts commit 9235e66.
677f896
to
3d8fd14
Compare
Hi there =) |
@Malven31 It may be worth looking into https://github.com/Gip-Gip/egui-plotter while we wait for this to be merged. I haven't tried it myself yet, but it looks promising. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks and works great! I left some comments on the code though, but nothing major
/// Set the x axis label of the main x-axis | ||
/// | ||
/// If no x-axis has been specified so far a new one will be created. | ||
pub fn x_axis_label(mut self, label: impl Into<WidgetText>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add an example of using these new functions to the doctest of Plot
(perhaps as a new example)!
This is great work, but i'm confused. Is it possible to keep the axis in old fashion way? it's quite useful for highly compact multiplots. |
egui::widgets::plot::Plot::show_axes
renamed toegui::widgets::plot::Plot::show_grid
. The originshow_axes
API now refers to the axis labelsAll better known plotting libraries like gnuplot, matlab, and also Implot draw the ticks outside of the plotting window:
egui paints the ticks inside the plotting window, which is unusual and, in my opinion, inconvenient.
In the same region, axis labels should be drawn.
CHANGELOG.md
under "Unreleased".egui_demo_lib
.cargo fmt
andcargo clippy
../scripts/check.sh
.