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

Program crashes when setting ctx.set_pixels_per_point #4187

Closed
RylanYancey opened this issue Mar 18, 2024 · 3 comments
Closed

Program crashes when setting ctx.set_pixels_per_point #4187

RylanYancey opened this issue Mar 18, 2024 · 3 comments
Labels
bug Something is broken

Comments

@RylanYancey
Copy link

RylanYancey commented Mar 18, 2024

Verify that issue has not been patched

  • Issue still exists when compiling with branch="master"
  • Issue exists regardless of using the 'wgpu' feature of eframe.

The Bug

  • Attempted to provide keyboard shortcut for changing the GUI Scale. (CNTR+ for scale up, CNTR- for scale down)
  • Implement by checking the context for input then updating the ctx.set_pixels_per_point(p)
  • Everything works fine when scaling up,
  • Program freezes on scale down, must be closed with CNTR+C,Force Quit, or xkill.

Desktop Information

  • OS: Linux / Ubuntu LTS 22.04
  • AMD Cpu + Nvidia Gpu
  • X11 display server

Related Code

This code runs at the end of every update. The issue persists when running at the beginning of every update.

        // if CNTR...
        if ctx.input(|i| i.modifiers.command) {
            ctx.input(|i| {
                // ...and +...
                if i.key_pressed(egui::Key::Plus) {
                    // ...increase pixels per point.
                    self.pixels_per_point += 0.1;
                    ctx.set_pixels_per_point(self.pixels_per_point);
                    return;
                }

                // ...and -...
                if i.key_pressed(egui::Key::Minus) {
                    // ...decrease pixels per point.
                    self.pixels_per_point -= 0.1;
                    // ACTUAL CRASH OCCURS IN THIS FUNCTION CALL
                    ctx.set_pixels_per_point(self.pixels_per_point);
                    return;
                }
            });
        }
@RylanYancey RylanYancey added the bug Something is broken label Mar 18, 2024
@YgorSouza
Copy link
Contributor

YgorSouza commented Mar 18, 2024

I think you just have a deadlock. As noted in input's documentation, you can't use the context inside the closure. Just move the set_pixels_per_point outside and it should work. In fact, the reason it seems to be working with + is because it is not running your code at all, Ctrl+ already defaults to zoom in egui so it consumes the shortcut, as you can see here:

pub mod kb_shortcuts {
use super::*;
/// Primary keyboard shortcut for zooming in (`Cmd` + `+`).
pub const ZOOM_IN: KeyboardShortcut = KeyboardShortcut::new(Modifiers::COMMAND, Key::Plus);
/// Secondary keyboard shortcut for zooming in (`Cmd` + `=`).
///
/// On an English keyboard `+` is `Shift` + `=`,
/// but it is annoying to have to press shift.
/// So most browsers also allow `Cmd` + `=` for zooming in.
/// We do the same.
pub const ZOOM_IN_SECONDARY: KeyboardShortcut =
KeyboardShortcut::new(Modifiers::COMMAND, Key::Equals);
/// Keyboard shortcut for zooming in (`Cmd` + `-`).
pub const ZOOM_OUT: KeyboardShortcut = KeyboardShortcut::new(Modifiers::COMMAND, Key::Minus);
/// Keyboard shortcut for resetting zoom in (`Cmd` + `0`).
pub const ZOOM_RESET: KeyboardShortcut = KeyboardShortcut::new(Modifiers::COMMAND, Key::Num0);
}
/// Let the user scale the GUI (change [`Context::zoom_factor`]) by pressing
/// Cmd+Plus, Cmd+Minus or Cmd+0, just like in a browser.
///
/// By default, [`crate::Context`] calls this function at the end of each frame,
/// controllable by [`crate::Options::zoom_with_keyboard`].
pub(crate) fn zoom_with_keyboard(ctx: &Context) {
if ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_RESET)) {
ctx.set_zoom_factor(1.0);
} else {
if ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_IN))
|| ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_IN_SECONDARY))
{
zoom_in(ctx);
}
if ctx.input_mut(|i| i.consume_shortcut(&kb_shortcuts::ZOOM_OUT)) {
zoom_out(ctx);
}
}
}

(I don't know why the same doesn't happen with Ctrl- though)

Edit: oh wait, you were probably testing by pressing Ctrl+= just like me, so that's why your code on the + branch wasn't running. It's not because of the default shortcut.

@RylanYancey
Copy link
Author

As @YgorSouza pointed out, a ctx cannot be modified from within the closure, something I must have missed. This is very unsafe, is there a way we could restrict the use of input so these sort of accidents' don't happen in the future?

@YgorSouza
Copy link
Contributor

Unfortunately, Rust does not consider deadlocks unsafe (nor does any language I think), and it's basically impossible to prevent them at compile time. The best we can do is avoid locks (and interior mutability in general) as much as possible, and document places where it can cause problems. There have been issues like this inside egui itself as well (like #2753). Maybe there is a better pattern that egui could have used, but no one has figured it out yet.

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

No branches or pull requests

2 participants