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

Fix panic when dragging window between monitors of different pixels_per_point #4088

Closed
wants to merge 35 commits into from

Conversation

rustbasic
Copy link
Contributor

@rustbasic rustbasic commented Feb 22, 2024

This is,

  1. When moving between screens with different pixels_per_point, you can move quickly (smooth).
  2. Prevents occasional panics (In fact, on very rare occasions a Viewport exit would cause a panic.).

@rustbasic rustbasic changed the title Pixels_per_point can move quickly when moving between different screens. When moving between screens with different pixels_per_point, you can move quickly. Feb 22, 2024
@emilk emilk changed the title When moving between screens with different pixels_per_point, you can move quickly. Fix panic when dragging window between monitors of different pixels_per_point Mar 12, 2024
Comment on lines +152 to +154
if self.0.len() <= idx.0 {
self.add(clip_rect, Shape::Noop);
}
Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't be needed unless the user does something wrong, and even then this only works if it is exactly off-by-one

Copy link
Contributor Author

@rustbasic rustbasic Mar 20, 2024

Choose a reason for hiding this comment

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

This shouldn't be needed unless the user does something wrong, and even then this only works if it is exactly off-by-one

I experienced a panic occurring when self.0.len() was 0, and there is no problem if I use this. glow_integration and wgpu_integration must also be changed. I will add that part to this commit as well.

How to reproduce panic:

  1. Run examples/test_viewports.
  2. Open all viewports.
  3. Close “DD -> Top D -> SS -> Top S”. (order required)

Copy link
Owner

Choose a reason for hiding this comment

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

I fail to reproduce.

In any case: try to find the root of the problem instead of adding defensive solutions

Copy link
Owner

Choose a reason for hiding this comment

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

And use RUST_BACKTRACE=1 to get stack traces

let texture_atlas = match ctx.fonts.get(&pixels_per_point.into()) {
Some(fonts) => fonts.texture_atlas(),
None => {
ctx.fonts.iter().next()
Copy link
Owner

@emilk emilk Mar 20, 2024

Choose a reason for hiding this comment

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

This will fall back to a texture atlas for the wrong pixels_per_point, which will likely look bad. The existing check is there to help users that write egui integration to find bugs in their code.

I'd rather figure out the root cause of the bug than mask it :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will fall back to a texture atlas for the wrong pixels_per_point, which will likely look bad. The existing check is there to help users that write egui integration to find bugs in their code.

I'd rather figure out the root cause of the bug than mask it :/

Don't worry, the texture atlas will be read for good pixel_per_point right away.
However, if you don't do this, a panic will occur before the texture atlas for a good pixel_per_point is read.
How to reproduce the panic is the same as the sequence shown above.

This also eliminates the root cause of the bug.
The root cause of another bug was fixed in #4128, so I didn't find it.

@rustbasic rustbasic requested a review from emilk March 20, 2024 14:39
@emilk emilk marked this pull request as draft March 25, 2024 12:16
@emilk
Copy link
Owner

emilk commented Apr 2, 2024

This is defensive programming that will hide the cause of the bugs instead of actually fixing them. Try to find the root of the problem instead.

@emilk emilk closed this Apr 2, 2024
@rustbasic rustbasic deleted the patch15 branch April 22, 2024 10:10
emilk added a commit that referenced this pull request Sep 17, 2024
…per_point (#4868)

Fix: panic when dragging window between monitors of different
pixels_per_point

This will continue to help us as we develop `egui`.
I hope you agree with my defense of `panic`.

* Relate #3959
* Relate #4088

* Closes #4178
* Closes #4179


There is also a way to add log if necessary.
```
                log::debug!("Anti-panic behavior occurs");
```

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants