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 work with bevy main (and eventually 0.8) #111

Merged
merged 6 commits into from
Jul 30, 2022

Conversation

DGriffin91
Copy link
Contributor

This needs a more thorough review. But it looks like the examples are working, and it's working in at least one of my projects.

@zimond
Copy link

zimond commented Jul 12, 2022

This is broken as bevy#4402 landed

@DGriffin91
Copy link
Contributor Author

Just messed with it a bit and it seems like this will require a bit of a more significant re-work now as the extract systems currently require mutable access to resources in the main world.

@zimond
Copy link

zimond commented Jul 13, 2022

Yes and using immutable Res with clone could make the examples run, but with only a black screen code I didn't have the time to dig in

@DGriffin91
Copy link
Contributor Author

DGriffin91 commented Jul 16, 2022

@zimond I tried using the clone with egui_render_output in the extract instead of take and it looks like it's working for me (at least with the included examples). I would assume the clone here is less efficient than take, but I'm not sure if it's enough slower to matter.

@zimond
Copy link

zimond commented Jul 17, 2022

I think some clones won't hurt the perf that much

@vladbat00
Copy link
Owner

vladbat00 commented Jul 17, 2022

Hi! Thank you for the PR! I think cloning is fine for now. Maybe, if we see an issue with performance, we might want to wrap it into a Mutex, if copying a lot of bytes around will become more expensive than dealing with locking.
I've also pushed a small commit to refactor the example, to make the error of initialization more visible (it'll panic now if we forget to initialize the camera properly, instead of making it silent) and fix the linters.

@zimond
Copy link

zimond commented Jul 18, 2022

Closing a window on this branch emits panic:

let extracted_window = &world.get_resource::<ExtractedWindows>().unwrap().windows[&self.window_id];
thread 'main' panicked at 'no entry found for key' egui_node.rs:328:14

@DGriffin91
Copy link
Contributor Author

Hmm, I tried this

        let extracted_windows = &world.get_resource::<ExtractedWindows>().unwrap().windows;

        let extracted_window =
            if let Some(extracted_window) = extracted_windows.get(&self.window_id) {
                extracted_window
            } else {
                return Ok(());
            };

But now when I close the window the app continues to run with the window closed and says:

2022-07-18T16:59:55.466789Z  INFO bevy_winit: Skipped event for closed window: WindowId(00000000-0000-0000-0000-000000000000)
2022-07-18T16:59:55.467187Z  INFO bevy_winit: Skipped event for closed window: WindowId(00000000-0000-0000-0000-000000000000)
2022-07-18T16:59:55.470299Z  INFO bevy_winit: Skipped event for closed window: WindowId(00000000-0000-0000-0000-000000000000)

@vladbat00
Copy link
Owner

Any chance that this is an issue on the Bevy's end? Not really sure what can cause this in bevy_egui itself.
I haven't got a chance to look into it myself, but does it happen with every example? Or is it specific to the multiple windows example?

If it happens in every example, I'd suggest to try to reproduce it in a clean Bevy project. And if it reproduces there without bevy_egui, we need to file an issue in their repo.

@DGriffin91
Copy link
Contributor Author

@mvlabat It appears that it is indeed an issue with bevy: bevyengine/bevy#5384

@CGMossa
Copy link

CGMossa commented Jul 24, 2022

Is this issue blocking this PR or has it been fixed?

@vladbat00
Copy link
Owner

Definitely not a blocker for us. I don't know if it has been fixed in Bevy's main, but we'll merge this PR regardless once 0.8 is released

@philpax
Copy link

philpax commented Jul 30, 2022

Bevy 0.8's now out - I've just built this fork with it (opened a PR for it here DGriffin91#2) and everything seems to work fine 👍

@vladbat00 vladbat00 marked this pull request as ready for review July 30, 2022 17:41
@vladbat00 vladbat00 merged commit 66bbed3 into vladbat00:main Jul 30, 2022
@vladbat00
Copy link
Owner

Thank you for your help with the update! I'll release it soon

@afonsolage afonsolage mentioned this pull request Jul 30, 2022
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