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.13 #236

Merged
merged 13 commits into from
Feb 19, 2024
Merged

Bevy 0.13 #236

merged 13 commits into from
Feb 19, 2024

Conversation

eerii
Copy link
Contributor

@eerii eerii commented Dec 21, 2023

Draft

Update to stay in sync with bevy main branch. Some changes are:

  • Input has been renamed to ButtonInput
  • create_bind_group_layout now takes two parameters instead of a struct
  • World queries are revamped and now split into QueryData and QueryFilter
  • Image now has a new RenderAssetPersistencePolicy field
  • EguiSettings::scale_factor changed type from f64 to f32 to match bevy's window_size

This pr is updated and tested until bevyengine/bevy@ebf81c6 (2024/02/14)

bevy-inspector-egui fork: https://github.com/eerii/bevy-inspector-egui

Relevant prs

src/egui_node.rs Outdated Show resolved Hide resolved
src/egui_node.rs Outdated Show resolved Hide resolved
@Shute052
Copy link

Shute052 commented Feb 3, 2024

The changes of the upstream PR (Bevy #10644) breaks the compilation of render_systems::setup_new_windows_render_system() on the latest version of bevy after the merge.

@Shute052
Copy link

Shute052 commented Feb 3, 2024

The changes of the upstream PR (Bevy #10644) breaks the compilation of render_systems::setup_new_windows_render_system() on the latest version of bevy after the merge.

/// Sets up the pipeline for newly created windows.
pub fn setup_new_windows_render_system(
    windows: Extract<Query<Entity, Added<Window>>>,
    mut render_graph: ResMut<RenderGraph>,
) {
    #[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
    struct EguiPass(String);

    for window in windows.iter() {
        let egui_pass = EguiPass(format!("egui-{}-{}", window.index(), window.generation()));

        let new_node = EguiNode::new(window);

        render_graph.add_node(egui_pass.clone(), new_node);

        render_graph.add_node_edge(bevy::render::graph::CameraDriverLabel, egui_pass);
    }
}

This works and successfully passed all of four tests in bevy_egui, which have limited coverage. I'm not entirely certain that all behaviors are error-free.

Update:

    #[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
    struct EguiPass(u32, u32);
    let egui_pass = EguiPass(window.index(), window.generation());

@DasLixou
Copy link

DasLixou commented Feb 3, 2024

The changes of the upstream PR (Bevy #10644) breaks the compilation of render_systems::setup_new_windows_render_system() on the latest version of bevy after the merge.

/// Sets up the pipeline for newly created windows.
pub fn setup_new_windows_render_system(
    windows: Extract<Query<Entity, Added<Window>>>,
    mut render_graph: ResMut<RenderGraph>,
) {
    #[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
    struct EguiPass(String);

    for window in windows.iter() {
        let egui_pass = EguiPass(format!("egui-{}-{}", window.index(), window.generation()));

        let new_node = EguiNode::new(window);

        render_graph.add_node(egui_pass.clone(), new_node);

        render_graph.add_node_edge(bevy::render::graph::CameraDriverLabel, egui_pass);
    }
}

This works and successfully passed all of four tests in bevy_egui, which have limited coverage. I'm not entirely certain that all behaviors are error-free.

I think it would be faster to make the label directly use index and generation, which would remove num to str conversion

@Vrixyz
Copy link
Contributor

Vrixyz commented Feb 3, 2024

updated to bevy main if that's useful: https://github.com/eerii/bevy_egui/pull/2

src/systems.rs Outdated
@@ -63,7 +63,7 @@ pub struct TouchId(pub Option<u64>);
pub struct InputResources<'w, 's> {
#[cfg(all(feature = "manage_clipboard", not(target_os = "android")))]
pub egui_clipboard: Res<'w, crate::EguiClipboard>,
pub keyboard_input: Res<'w, Input<KeyCode>>,
pub keyboard_input: Res<'w, ButtonInput<KeyCode>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to read the event KeyboardInput, as KeyCode in bevy is no longer layout dependant.

Maybe it could be done in a followup issue?

Copy link
Owner

Choose a reason for hiding this comment

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

@Vrixyz do we want these inputs to be layout dependent? We are reading them only to check key modifiers (such as shift/ctrl/etc).

Copy link

Choose a reason for hiding this comment

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

Modifiers aren't exempt from keyboard layout, prominently many layouts have AltGraph instead of AltRight. Less industry-standard but still out there Neo replaces capslock and the key right of left shift (that US layouts don't have) with modifiers.

It may not have a huge practical impact and people with those kinds of layout are already accustomed to additional pain (compared to say plain dvorak) but consistently using logical mappings sounds like a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eerii you might be interested in parts of that diff: Vrixyz@86650bb ; I think that's not the whole diff needed to use logical key everywhere, but it was enough to get me unlock on my particular issue.

I look forward to finally consolidate bevy_egui to non-qwerty layouts :D

(Just when I ordered a qwerty keyboard 🥲)

Copy link
Contributor Author

@eerii eerii Feb 14, 2024

Choose a reason for hiding this comment

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

I updated the branch to match main and to track the latest bevy main, and added your diff. It seems to work and while I'm not sure where else to look to migrate logical input it seems an improvement! Thanks ^^
Edit: Not sure how to change the keycode into a keyboardinto yet, I will look into it

Copy link
Owner

Choose a reason for hiding this comment

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

It's a bit of a shame that Bevy 0.13 doesn't have ButtonInput<Key> for logical keys

Copy link
Owner

Choose a reason for hiding this comment

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

I've just re-implemented the modifiers logic with logical key codes, I'll keep this for review till the evening and then merge it. I also hope to tackle the other input-related issues shortly after and then release a new version.

@Plonq Plonq mentioned this pull request Feb 17, 2024
1 task
This was referenced Feb 18, 2024
@sclausen sclausen mentioned this pull request Feb 18, 2024
examples/render_to_image_widget.rs Outdated Show resolved Hide resolved
examples/render_to_image_widget.rs Outdated Show resolved Hide resolved
examples/side_panel.rs Outdated Show resolved Hide resolved
examples/side_panel.rs Outdated Show resolved Hide resolved
@vladbat00 vladbat00 marked this pull request as ready for review February 19, 2024 18:29
@vladbat00 vladbat00 merged commit eb3959f into vladbat00:main Feb 19, 2024
39 checks passed
@vladbat00
Copy link
Owner

Thank you @eerii and everyone else involved! ❤️

ManevilleF added a commit to ManevilleF/hexx that referenced this pull request Feb 21, 2024
ManevilleF added a commit to ManevilleF/bevy_silk that referenced this pull request Feb 22, 2024
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.

7 participants