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

update to bevy 0.5 #12

Merged
merged 28 commits into from
Apr 10, 2021
Merged

update to bevy 0.5 #12

merged 28 commits into from
Apr 10, 2021

Conversation

jakobhellermann
Copy link
Contributor

This shouldn't be merged yet, I'm just putting this up as a draft PR so that anyone who wants to update to bevy main can see that the work is already done.

includes #3

src/lib.rs Outdated Show resolved Hide resolved
this way, when patching bevy using [patch.crates-io] only one crate
needs to be patched and no confusing "expected WinitPlugin, got
WinitPlugin" errors occur
src/lib.rs Show resolved Hide resolved
this should solve the wasm_bindgen version selection
failures and also allows `cargo run --example` without features.
@jakobhellermann jakobhellermann marked this pull request as ready for review April 3, 2021 19:09
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/egui_node.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
src/transform_node.rs Outdated Show resolved Hide resolved
Co-authored-by: Vladyslav Batyrenko <mvlabat@gmail.com>
@mvlabat
Copy link
Owner

mvlabat commented Apr 7, 2021

I believe that at the moment bevy_egui doesn't work with bevy_webgl2 since it's not yet upgraded to 0.5. I'd like to hold off the release of 0.5 support until bevy_webgl2 is upgraded, but I'm fine with merging this to master earlier. So at least it'll be possible for users to point dependency resolution to this repo instead of the fork.

@jakobhellermann
Copy link
Contributor Author

Alright, bevy_webgl2 shouldn't take too long to release, IIRC it is already ported to bevy main and just needs to be published.

@mvlabat
Copy link
Owner

mvlabat commented Apr 9, 2021

Thanks for the egui upgrade! I'm going to test and merge this tomorrow :)

@mvlabat
Copy link
Owner

mvlabat commented Apr 10, 2021

@jakobhellermann I believe there's a regression with users' textures rendering. See the ui example, now the Bevy logo isn't displayed (it should be in the left panel, like on the screenshot in the readme). I'm also not able to work on any Bevy 0.5 project after wgpu was upgraded to 0.7. I'm on Windows, here every project crashes for me on startup now: gfx-rs/wgpu#1317 :(

I can test things on my old Macbook, but it takes forever to compile... So I won't be able to help with debugging very actively.

@mvlabat
Copy link
Owner

mvlabat commented Apr 10, 2021

Ok, now it doesn't crash but hangs. :)
(Now it's only bevy_egui, other Bevy examples work fine.)

src/systems.rs Outdated Show resolved Hide resolved
@jakobhellermann
Copy link
Contributor Author

The texture ui seems to have been broken by the egui 0.11 upgrade, I'll look into it.

I also noticed that sometimes minimizing an egui window will lead to a

wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(1, 114, Vulkan)>`
    In a set_scissor_rect command
    Invalid ScissorRect parameters

@jakobhellermann
Copy link
Contributor Author

The ui is broken since emilk/egui@589bae1.

Removing the left to right layout in

ui.with_layout(egui::Layout::left_to_right(), |ui| {
fixes the problem.

@jakobhellermann
Copy link
Contributor Author

I opened an issue: emilk/egui#289

@mvlabat
Copy link
Owner

mvlabat commented Apr 10, 2021

The ui is broken since emilk/egui@589bae1.

Removing the left to right layout in

ui.with_layout(egui::Layout::left_to_right(), |ui| {

fixes the problem.

It seems like doing

ui.allocate_space(egui::Vec2::new(1.0, 100.0));
ui.horizontal(|ui| {
    load = ui.button("Load").clicked();
    invert = ui.button("Invert").clicked();
    remove = ui.button("Remove").clicked();
});

does the trick for us as well.

@mvlabat
Copy link
Owner

mvlabat commented Apr 10, 2021

Looks great! Thanks again for your work on this

@mvlabat mvlabat merged commit 58e7c17 into mvlabat:main Apr 10, 2021
@jakobhellermann
Copy link
Contributor Author

The texture ui seems to have been broken by the egui 0.11 upgrade, I'll look into it.

I also noticed that sometimes minimizing an egui window will lead to a

wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(1, 114, Vulkan)>`
    In a set_scissor_rect command
    Invalid ScissorRect parameters

This is reproducible using

    egui::Window::new("Window")
        .scroll(true)
        .show(&egui_context.ctx, |ui| {
            ui.label("Windows can be moved by dragging them.");
            ui.label("They are automatically sized based on contents.");
            ui.label("You can turn on resizing and scrolling if you like.");
            ui.label("You would normally chose either panels OR windows.");
        });

@jakobhellermann
Copy link
Contributor Author

Can I also suggest one change before releasing, add a egui_context.ctx() function and make the ctx field private. That way #13 can be done without a breaking change.

@jakobhellermann jakobhellermann deleted the bevy-0.5 branch April 10, 2021 11:45
@jakobhellermann jakobhellermann restored the bevy-0.5 branch April 10, 2021 11:45
@mvlabat
Copy link
Owner

mvlabat commented Apr 10, 2021

Yeah, I even hope we can merge #13 before the major release. :)

@mvlabat mvlabat mentioned this pull request Apr 10, 2021
@jakobhellermann jakobhellermann deleted the bevy-0.5 branch April 10, 2021 12:03
@mvlabat
Copy link
Owner

mvlabat commented Apr 10, 2021

This is reproducible using

    egui::Window::new("Window")
        .scroll(true)
        .show(&egui_context.ctx, |ui| {
            ui.label("Windows can be moved by dragging them.");
            ui.label("They are automatically sized based on contents.");
            ui.label("You can turn on resizing and scrolling if you like.");
            ui.label("You would normally chose either panels OR windows.");
        });

I've just stumbled upon this as well. I believe we should file an issue to egui about this. It sometimes returns negatives for clipping zone height.

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.

4 participants