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

Improved plot groups and bounds handling #2410

Merged
merged 32 commits into from
Apr 18, 2023

Conversation

s-nie
Copy link
Contributor

@s-nie s-nie commented Dec 8, 2022

Closes #2409

@s-nie
Copy link
Contributor Author

s-nie commented Dec 15, 2022

This allowed me to do some multi-threaded window drawing where I stumbled across a couple of deadlocks. I fixed them here as well, but let me know if you'd prefer a separate PR for that.

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 good!

CHANGELOG.md Outdated Show resolved Hide resolved
crates/egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved
crates/egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved
crates/egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved
crates/egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved
crates/egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved
crates/egui_demo_lib/src/demo/plot_demo.rs Outdated Show resolved Hide resolved
s-nie and others added 5 commits January 23, 2023 12:11
@s-nie
Copy link
Contributor Author

s-nie commented Jan 23, 2023

Thanks for taking the time @emilk. I addressed the comments. It looks like CI is failing, but for a reason unrelated to this PR.

@s-nie s-nie requested a review from emilk January 25, 2023 11:18
@s-nie s-nie mentioned this pull request Jan 30, 2023
@emilk
Copy link
Owner

emilk commented Feb 4, 2023

I think this broke something:

bad-groups

@s-nie s-nie force-pushed the plot-groups-and-bounds-improvements branch from fee22a7 to e39fd37 Compare February 4, 2023 19:40
@s-nie
Copy link
Contributor Author

s-nie commented Feb 5, 2023

@emilk Good catch, d8efd9a should fix it.

CHANGELOG.md Outdated Show resolved Hide resolved
response: Response,
bounds_modifications: Vec<BoundsModification>,
Copy link
Owner

Choose a reason for hiding this comment

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

why are you accumulating modifications instead of applying them right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that they can be applied together at the right time, there are some other transforms that should be done between lines 711 and 763 before the modifications.

I'm sure there are other ways to do this as well, but the bound handling is pretty complex and I didn't want to mess with it too much.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you add it as a comment to the code? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment in eaec082.

@OmegaJak
Copy link
Contributor

I made some improvements to the interaction with auto_bounds in a PR on the source repo: s-nie#1

We're in fork-of-a-fork territory here to if there's a better way to handle it than making a PR on the fork's repo, then please let me know!

@s-nie
Copy link
Contributor Author

s-nie commented Apr 14, 2023

Thanks for the improvement @OmegaJak! I merged it into this branch.

@emilk emilk merged commit 69b568a into emilk:master Apr 18, 2023
@s-nie s-nie deleted the plot-groups-and-bounds-improvements branch April 18, 2023 14:32
@emilk emilk added egui_plot Related to egui_plot egui labels Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui_plot Related to egui_plot egui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve implementation of linked plot bounds and cursors
3 participants