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

Bevy 0.10 #159

Merged
merged 22 commits into from
Mar 8, 2023
Merged

Bevy 0.10 #159

merged 22 commits into from
Mar 8, 2023

Conversation

mvlabat
Copy link
Owner

@mvlabat mvlabat commented Mar 6, 2023

Based on #148

@mvlabat mvlabat self-assigned this Mar 6, 2023
examples/ui.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@TimJentzsch
Copy link

TimJentzsch commented Mar 7, 2023

Ci seems to complain about a mutable reference in pipelines.specialize:
https://github.com/mvlabat/bevy_egui/pull/159/files#diff-ab44fb5ff047491ab87031ab0d263274a3deb0951822df16d759522ac9b44fa8L256

@mvlabat
Copy link
Owner Author

mvlabat commented Mar 7, 2023

I'm seeing the following warnings in the ui example:

epaint: WARNING: pixels_per_point (dpi scale) have changed between text layout and tessellation. You must recreate your text shapes if pixels_per_point changes.

I'd be grateful if anyone could pick up this issue. I won't have time to return to the PR until the evening.

@DGriffin91
Copy link
Contributor

I've been trying to track down the pixels_per_point thing. Haven't figured it out yet. This is the commit that added the warning:
emilk/egui@39b63f6

In the ui example, it comes from changing the egui_settings.scale_factor in update_ui_scale_factor_system()

We then set egui_input.pixels_per_point in update_window_contexts() which is called from process_input_system()

We get the shapes in process_output_system()

let full_output = ctx.end_frame();
let egui::FullOutput {
    platform_output,
    shapes,
    textures_delta,
    repaint_after,
} = full_output;

egui_render_output.shapes = shapes;

I think it only shows the warning for one frame if scale_factor changes.

Comment on lines 14 to 15
fn ui_example_system(mut egui_ctx: Query<&mut EguiContext, With<PrimaryWindow>>) {
egui::Window::new("Hello").show(egui_ctx.single_mut().get_mut(), |ui| {
Copy link
Contributor

Choose a reason for hiding this comment

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

can it be simplified like this?

Suggested change
fn ui_example_system(mut egui_ctx: Query<&mut EguiContext, With<PrimaryWindow>>) {
egui::Window::new("Hello").show(egui_ctx.single_mut().get_mut(), |ui| {
fn ui_example_system(egui_ctx: Query<&EguiContext, With<PrimaryWindow>>) {
egui::Window::new("Hello").show(egui_ctx.single(), |ui| {

Copy link
Owner Author

Choose a reason for hiding this comment

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

Only if we implement Deref and DerefMut for EguiContext, giving people an interface to borrow the context immutably, which I'm hesitant to do due to the following:

bevy_egui/src/lib.rs

Lines 247 to 256 in 7151c40

/// Even though the mutable borrow isn't necessary, as the context is wrapped into `RwLock`,
/// using the immutable getter is gated with the `immutable_ctx` feature. Using the immutable
/// borrow is discouraged as it may cause unpredictable blocking in UI systems.
///
/// When the context is queried with `&mut EguiContext`, the Bevy scheduler is able to make
/// sure that the context isn't accessed concurrently and can perform other useful work
/// instead of busy-waiting.
pub fn get_mut(&mut self) -> &mut egui::Context {
&mut self.0
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

I ended up introducing the EguiContexts system param, to combine the best of both worlds: low boilerplate and mutable borrowing by default.

@mvlabat
Copy link
Owner Author

mvlabat commented Mar 7, 2023

@DGriffin91 thanks for looking into this! That definitely saved me time investigating. It seems that I was able to fix the warning here: 7151c40.

@mvlabat
Copy link
Owner Author

mvlabat commented Mar 8, 2023

The PR is almost finished. I'll let people review the API changes, and tomorrow morning I'll push the last bits of the documentation fixes and make a release.

@mvlabat mvlabat merged commit bb41bbc into main Mar 8, 2023
@mvlabat mvlabat deleted the bevy-0.10 branch March 8, 2023 11:17
@mvlabat mvlabat mentioned this pull request Mar 8, 2023
StrikeForceZero pushed a commit to StrikeForceZero/bevy_egui that referenced this pull request Jul 5, 2024
Co-authored-by: Jakob Hellermann <jakob.hellermann@protonmail.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
Development

Successfully merging this pull request may close these issues.

6 participants