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

Upgrade to Bevy 0.14 & upgrade egui-gizmo dependency #110

Merged
merged 31 commits into from
Aug 11, 2024

Conversation

zhaop
Copy link
Contributor

@zhaop zhaop commented Jul 6, 2024

I am no longer working on this PR due to lack of time. I'm keeping it open for now for visibility -- feel free to make your own changes and make it your own PR.

It basically works, except for one problem where certain windows would keep growing in height.


This PR allows bevy_editor_pls to be used with Bevy 0.14, and is based on top of #106 by @ActuallyHappening .

Before anything happens to this, we should probably first:


Concretely, this implements the following migration steps:

and also fixes an example:

ActuallyHappening and others added 19 commits June 9, 2024 20:49
but a bit buggy
accidentally committed changes
and added documentation here and there
@zhaop zhaop mentioned this pull request Jul 6, 2024
@zhaop
Copy link
Contributor Author

zhaop commented Jul 6, 2024

@jakobhellermann The changes for bevy 0.14 are done here -- this is just waiting on a few dependencies to get published to crates.io (& PR #106).

FYI commit e9bec52 also bumps the version number to 0.9.0 & adds corresponding entries to README, CHANGELOG -- but let me know if you want me to revert that.

@jakobhellermann
Copy link
Owner

Thanks a lot. I'll update bevy_mod_debugdump and then we can wait for urholaukkarinen/transform-gizmo#69 and merge + release

@jakobhellermann
Copy link
Owner

bevy_mod_debugdump is updated

@zhaop
Copy link
Contributor Author

zhaop commented Jul 7, 2024

Hmm just noticed a few more things: forgot about the examples 😅 but more seriously, opening any editor window results in:

thread 'main' panicked at /Users/patrick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/egui-0.28.1/src/placer.rs:109:9:
assertion failed: child_size.is_finite() && child_size.x >= 0.0 && child_size.y >= 0.0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_editor_pls_core::editor::Editor::system`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

which is caused by bevy_editor_pls/crates/bevy_editor_pls_core/src/editor.rs:460:

ui.allocate_space(ui.available_size() - (5.0, 5.0).into());

It sometimes requests space with negative width, which was "tolerated" before egui 0.28, but since egui #4478 is now an assert that panics. I'm not so familiar with how this crate handles sizes, so I'm not sure how to deal with this yet. Any ideas?

@jakobhellermann
Copy link
Owner

I'd try replacing the -5 with something like a saturating_sub or an if to make sure it never goes below zero.

@zhaop
Copy link
Contributor Author

zhaop commented Jul 7, 2024

Hmm, changing it to

let desired_size = (ui.available_size() - (5.0, 5.0).into()).max((0.0, 0.0).into());
ui.allocate_space(desired_size);

where desired_size.y used to be -5 and is now 0, leads to an ever growing window height (here on the "empty" example):

expanding-window.mp4

For context, here is the surrounding for loop:

        for (i, floating_window) in floating_windows.into_iter().enumerate() {
            let id = egui::Id::new(floating_window.id);
            let title = self.windows[&floating_window.window].name;

            let mut open = true;
            let default_size = self.windows[&floating_window.window].default_size;
            let mut window = egui::Window::new(title)
                .id(id)
                .open(&mut open)
                .resizable(true)
                .default_size(default_size);
            if let Some(initial_position) = floating_window.initial_position {
                window = window.default_pos(initial_position - egui::Vec2::new(10.0, 10.0))
            }
            window.show(ctx, |ui| {
                self.editor_window_inner(world, internal_state, floating_window.window, ui);
                let desired_size = (ui.available_size() - (5.0, 5.0).into()).max((0.0, 0.0).into());
                ui.allocate_space(desired_size);
            });

            if !open {
                close_floating_windows.push(i);
            }
        }

@zhaop
Copy link
Contributor Author

zhaop commented Jul 7, 2024

The examples are now migrated, cargo test no longer shows errors, everything compiles.

Some examples are a bit broken, although this was already the case on main:

  • separate_window panics in wgpu:
thread 'main' panicked at /Users/patrick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wgpu-0.20.1/src/backend/wgpu_core.rs:2996:5:
wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `main_opaque_pass_3d_command_encoder`
    In a set_viewport command
    Viewport has invalid rect Rect { x: 1501.0, y: 92.0, w: 2560.0, h: 1440.0 }; origin and/or size is less than or equal to 0, and/or is not contained in the render target (2560, 1440, 1)


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at /Users/patrick/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_tasks-0.14.0/src/single_threaded_task_pool.rs:144:54:
called `Option::unwrap()` on a `None` value
Encountered a panic in system `bevy_render::renderer::render_system`!
  • ui is missing an asset at bevy_editor_pls/crates/bevy_editor_pls/assets/branding/bevy_logo_dark_big.png

@zhaop
Copy link
Contributor Author

zhaop commented Jul 12, 2024

Hmm, I'm not sure I have time to debug the growing-window-height issue above. Does anyone else with more experience with egui have an idea how to resolve this?

@Aceeri
Copy link
Contributor

Aceeri commented Jul 13, 2024

I can't seem to repro the issue, you said using the Open Window would cause it right?

@zhaop
Copy link
Contributor Author

zhaop commented Jul 13, 2024

@Aceeri Could you give these steps a try:

git clone [email protected]:zhaop/bevy_editor_pls.git --branch bevy-0.14
cd bevy_editor_pls/crates/bevy_editor_pls
cargo run --example empty

Then, click "Open window", then click "Hierarchy".

What I see: the Hierarchy window shows up, and its height continuously grows until filling the whole window height.

What I expected to see: the Hierarchy window shows up at a certain size and stays the same size.

@Aceeri
Copy link
Contributor

Aceeri commented Jul 13, 2024

@Aceeri Could you give these steps a try:

git clone [email protected]:zhaop/bevy_editor_pls.git --branch bevy-0.14
cd bevy_editor_pls/crates/bevy_editor_pls
cargo run --example empty

Then, click "Open window", then click "Hierarchy".

What I see: the Hierarchy window shows up, and its height continuously grows until filling the whole window height.

What I expected to see: the Hierarchy window shows up at a certain size and stays the same size.

Gotcha, I can see it now

@XX
Copy link

XX commented Aug 10, 2024

@zhaop What is the status of this PR? Is help needed?

@zhaop
Copy link
Contributor Author

zhaop commented Aug 10, 2024

@XX I'd be happy if someone could take over this PR -- I don't think I'll have time to work on this anymore (and have moved away from using this crate in my project).

As I understand, it basically works, except for that one problem where the height of certain windows keeps growing when it shouldn't.

@jakobhellermann
Copy link
Owner

I think I figured out a fix for that, the windows where this was a problem had .auto_shrink([false, false]), removing that make them not grow automatically anymore: 8ae7170

@jakobhellermann
Copy link
Owner

I think this is good to merge now, thank you very much for the PR @zhaop and everyone for the patience, I'm gonna merge this now and release a new version

@jakobhellermann jakobhellermann marked this pull request as ready for review August 11, 2024 09:42
@jakobhellermann jakobhellermann merged commit 6cfd972 into jakobhellermann:main Aug 11, 2024
@zhaop
Copy link
Contributor Author

zhaop commented Aug 11, 2024

Thanks a lot @jakobhellermann for the fix :)

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.

5 participants