Skip to content

Commit

Permalink
Fix: Window position creeps between executions on scaled monitors (#4443
Browse files Browse the repository at this point in the history
)

<!--
Please read the "Making a PR" section of
[`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUTING.md)
before opening a Pull Request!

* Keep your PR:s small and focused.
* The PR title is what ends up in the changelog, so make it descriptive!
* If applicable, add a screenshot or gif.
* If it is a non-trivial addition, consider adding a demo for it to
`egui_demo_lib`, or a new example.
* Do NOT open PR:s from your `master` branch, as that makes it hard for
maintainers to add commits to your PR.
* Remember to run `cargo fmt` and `cargo clippy`.
* Open the PR as a draft until you have self-reviewed it and run
`./scripts/check.sh`.
* When you have addressed a PR comment, mark it as resolved.

Please be patient! I will review your PR, but my time is limited!
-->

* Closes <#4442>
* Refactors active monitor detection so it can be called from multiple
locations.

Compare this gif to the one on the issue report.

![egui_window_position_no_creep](https://github.com/emilk/egui/assets/45777186/8e05d4fb-266e-48b9-9223-b65f16500a99)

### Investigation notes

- [`WindowSettings.inner_position_pixels` and
`WindowSettings.outer_position_pixels`](https://github.com/emilk/egui/blob/master/crates/egui-winit/src/window_settings.rs#L8-L12)
are stored in physical/pixel coordinates.
- `ViewportBuilder::with_position` expects to be passed a position in
_logical_ coordinates.
- Prior to this PR, the position was being passed from `WindowSettings`
to `with_position` [without any
scaling](https://github.com/emilk/egui/blob/master/crates/egui-winit/src/window_settings.rs#L61-L68).
This was the root cause of the issue.
- The fix is to first convert the position to logical coordinates,
respecting the scaling factor of the active monitor. This requires us to
first determine the active monitor, so I factored out some of the logic
in
[`clamp_pos_to_monitor`](https://github.com/emilk/egui/blob/master/crates/egui-winit/src/window_settings.rs#L130)
to find the active monitor.
  • Loading branch information
avery-radmacher authored May 11, 2024
1 parent 66d2b3f commit e06b225
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 7 deletions.
6 changes: 5 additions & 1 deletion crates/eframe/src/native/epi_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ pub fn viewport_builder<E>(
}
window_settings.clamp_position_to_monitors(egui_zoom_factor, event_loop);

viewport_builder = window_settings.initialize_viewport_builder(viewport_builder);
viewport_builder = window_settings.initialize_viewport_builder(
egui_zoom_factor,
event_loop,
viewport_builder,
);
window_settings.inner_size_points()
} else {
if let Some(pos) = viewport_builder.position {
Expand Down
39 changes: 33 additions & 6 deletions crates/egui-winit/src/window_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ impl WindowSettings {
self.inner_size_points
}

pub fn initialize_viewport_builder(
pub fn initialize_viewport_builder<E>(
&self,
egui_zoom_factor: f32,
event_loop: &winit::event_loop::EventLoopWindowTarget<E>,
mut viewport_builder: ViewportBuilder,
) -> ViewportBuilder {
crate::profile_function!();
Expand All @@ -64,7 +66,15 @@ impl WindowSettings {
self.outer_position_pixels
};
if let Some(pos) = pos_px {
viewport_builder = viewport_builder.with_position(pos);
let monitor_scale_factor = if let Some(inner_size_points) = self.inner_size_points {
find_active_monitor(egui_zoom_factor, event_loop, inner_size_points, &pos)
.map_or(1.0, |monitor| monitor.scale_factor() as f32)
} else {
1.0
};

let scaled_pos = pos / (egui_zoom_factor * monitor_scale_factor);
viewport_builder = viewport_builder.with_position(scaled_pos);
}

if let Some(inner_size_points) = self.inner_size_points {
Expand Down Expand Up @@ -127,12 +137,12 @@ impl WindowSettings {
}
}

fn clamp_pos_to_monitors<E>(
fn find_active_monitor<E>(
egui_zoom_factor: f32,
event_loop: &winit::event_loop::EventLoopWindowTarget<E>,
window_size_pts: egui::Vec2,
position_px: &mut egui::Pos2,
) {
position_px: &egui::Pos2,
) -> Option<winit::monitor::MonitorHandle> {
crate::profile_function!();

let monitors = event_loop.available_monitors();
Expand All @@ -142,7 +152,7 @@ fn clamp_pos_to_monitors<E>(
.primary_monitor()
.or_else(|| event_loop.available_monitors().next())
else {
return; // no monitors 🤷
return None; // no monitors 🤷
};

for monitor in monitors {
Expand All @@ -159,6 +169,23 @@ fn clamp_pos_to_monitors<E>(
}
}

Some(active_monitor)
}

fn clamp_pos_to_monitors<E>(
egui_zoom_factor: f32,
event_loop: &winit::event_loop::EventLoopWindowTarget<E>,
window_size_pts: egui::Vec2,
position_px: &mut egui::Pos2,
) {
crate::profile_function!();

let Some(active_monitor) =
find_active_monitor(egui_zoom_factor, event_loop, window_size_pts, position_px)
else {
return; // no monitors 🤷
};

let mut window_size_px =
window_size_pts * (egui_zoom_factor * active_monitor.scale_factor() as f32);
// Add size of title bar. This is 32 px by default in Win 10/11.
Expand Down

0 comments on commit e06b225

Please sign in to comment.