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

Plot auto-fit broken since 0.17.x #1649

Closed
cessen opened this issue May 19, 2022 · 9 comments · Fixed by #1865
Closed

Plot auto-fit broken since 0.17.x #1649

cessen opened this issue May 19, 2022 · 9 comments · Fixed by #1865
Labels
bug Something is broken

Comments

@cessen
Copy link

cessen commented May 19, 2022

Describe the bug

In egui 0.16.x and earlier, plots would by default auto-fit to ensure that the entire plot was visible, like this:

This would work regardless of the data aspect ratio or the aspect ratio of the containing frame. However, since 0.17.x (up to and including 0.18.0 and current master as of filing this issue), if the plot frame has a wider aspect ratio than the data, the auto-fitting cuts off the top and bottom of the plot like this:

It seems like the code is only fitting for width, but not for height. This erroneous behavior also manifests with the Plot::include_x() and Plot::include_y() methods, making the bug difficult to work around manually in client code.

To Reproduce

  1. Build and run https://github.com/cessen/egui_plot_fit_bug
  2. Play with the window aspect ratio and watch as it fails to auto-fit the diagonal line plot.

Desktop:

  • OS: NixOS 21.11 x86-64, running X11 and AwesomeWM
@cessen cessen added the bug Something is broken label May 19, 2022
@enomado
Copy link
Contributor

enomado commented May 20, 2022

i'll check this. Looks like I broke it.

@enomado
Copy link
Contributor

enomado commented May 20, 2022

Just checked. Not me.

Could you create example on 0.17? It always have been prioritizing X axis - https://github.com/emilk/egui/blob/master/egui/src/widgets/plot/mod.rs#L722

You could try to pass true there, or try to find out what axis should be preserved according to that overflow

@enomado
Copy link
Contributor

enomado commented May 20, 2022

I believe you want add this lines

let frame_aspect = self.frame.aspect_ratio();
let preserve_y = frame_aspect > aspect as f32;

here https://github.com/emilk/egui/blob/master/egui/src/widgets/plot/transform.rs#L316

@cessen
Copy link
Author

cessen commented May 23, 2022

Could you create example on 0.17?

Here's the same example for 0.17:
https://github.com/cessen/egui_plot_fit_bug/tree/egui-0.17

And for 0.16:
https://github.com/cessen/egui_plot_fit_bug/tree/egui-0.16

The relevant code is identical in all cases--only the app setup boilerplate is different.

EDIT: and just to be clear, 0.16 has the correct expected behavior. The bug was introduced in 0.17.

cessen added a commit to EatTheFuture/image_tools that referenced this issue May 24, 2022
@DusterTheFirst
Copy link
Contributor

DusterTheFirst commented Jun 7, 2022

I have also noticed the auto fit also does not like lines or points with a constant y value. Here is a demo of this in action:

recordedVideo.mp4

You can see the auto fit work perfectly fine with the very noisy infrared:test signal, and even with both the infrared:test and infrared:rock.diff signals. But when the only signal is the constant infrared:rock.diff signal, the auto fit just snaps to the origin. I show that the line is still there, just not being auto fit. At the end, I double click to return to auto fit, and once again the plot snaps to the origin.

I have checked and this behavior happens even with infrared:rock.diff being a non-zero constant as long as there is no include_y with a value other than the constant one being plotted.

The work around to fix this is to just add two include_y's to give the plot a minimum height, but this prevents the auto-fit from zooming in on the Y axis any smaller than that minimum height which can be unwanted.

I have also seen this occur when the plot only contains one point on the x axis. It looks to be the same issue as with the Y axis

@cessen
Copy link
Author

cessen commented Jun 17, 2022

@enomado, @emilk

Any progress on this? It's not super urgent, but it is blocking the next public release of a set of tools I maintain.

(I've attempted to hack around this in the client code, and I've successfully made it frame things properly, but it also snaps back to auto-framing whenever the plotted data changes, which makes it essentially impossible for the user to manually re-frame things when the data is continually changing (which is a common situation in these tools).)

@cessen
Copy link
Author

cessen commented Jul 27, 2022

@emilk @enomado

Any word on this yet? Even just "it's not a priority, and we have no timeline for when we might fix this" would be useful information to have, so I can figure out another way to prep for the next release of the tools I maintain (which we'd like to do soon, at this point).

@emilk
Copy link
Owner

emilk commented Jul 29, 2022

I'll take a look

@cessen
Copy link
Author

cessen commented Jul 29, 2022

Awesome, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants