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 for bevy 0.10 #148

Closed
wants to merge 16 commits into from
Closed

Update for bevy 0.10 #148

wants to merge 16 commits into from

Conversation

DGriffin91
Copy link
Contributor

Having a go at trying to update this for current bevy main. Tried to use the window Entity as the window id, but running into the issue that it is currently designed expecting that the id for the primary window will already exist before the window does.

Most relevant PRs to look at:
Windows as Entities #5589
Migrate engine to Schedule v3 #7267

@DGriffin91
Copy link
Contributor Author

DGriffin91 commented Feb 21, 2023

Initially got the examples working with some string and bubblegum. Wondering if the instances of resources with HashMap<Entity, Something> should now be components added to the window?

@DGriffin91 DGriffin91 changed the title Update for bevy main Update for bevy 0.10 Mar 4, 2023
@@ -171,9 +175,9 @@ pub struct EguiNode {

impl EguiNode {
/// Constructs Egui render node.
pub fn new(window_id: WindowId) -> Self {
pub fn new(window_entity: Entity) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to update to bevy main before seeing your PR ; and I did a type WindowEntity = Entity, to help me understand where the various Entity references were supposed to reference a window.

Would that be interesting ? It might be discussed in another issue, upgrading can already be a big enough scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could potentially help make things a bit more clear, though I wonder if we should really be storing the entity all over the place vs turning most things into components on the window entity.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd like us to try the components approach. Not sure what that would look like exactly, but since windows are entities now, storing additional data as components sounds more idiomatic

Fix build error from missing add_systems_to_schedule
) {
let cube_preview_texture_id = egui_ctx.image_id(&cube_preview_image).unwrap();
let preview_material_handle = preview_cube_query.single();
let preview_material = materials.get_mut(preview_material_handle).unwrap();

let ctx = egui_ctx.ctx_mut();
let ctx = egui_ctx.ctx_for_window_mut(windows.iter().next().unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a loss of ergonomics indeed, I look forward to what can be done component-wise :) ; egui_context stored in a component attached to a window is my first not-much-informed thought

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now this looks like:

egui_ctx: Query<&EguiContext, With<Window>>,
[...]
let ctx = egui_ctx.iter().next().unwrap();

Thoughts?
The With<Window> isn't really needed, but maybe clarifies things. idk if it makes sense or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

With<PrimaryWindow> might convey a better intention, that way you can use egui_ctx.single() ?

@mvlabat mvlabat mentioned this pull request Mar 7, 2023
@mvlabat
Copy link
Owner

mvlabat commented Mar 8, 2023

Thanks a lot for the help! I merged the update as part of #159, so closing this one.

@mvlabat mvlabat closed this Mar 8, 2023
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