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

Arc WinitWindow and extract to render world #12524

Closed
wants to merge 15 commits into from
2 changes: 2 additions & 0 deletions crates/bevy_internal/Cargo.toml
Expand Up @@ -171,6 +171,8 @@ bevy_dev_tools = ["dep:bevy_dev_tools"]
# Enable support for the ios_simulator by downgrading some rendering capabilities
ios_simulator = ["bevy_pbr?/ios_simulator", "bevy_render?/ios_simulator"]

bevy_winit = ["dep:bevy_winit", "bevy_render?/bevy_winit"]

[dependencies]
# bevy
bevy_a11y = { path = "../bevy_a11y", version = "0.14.0-dev" }
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_render/Cargo.toml
Expand Up @@ -57,6 +57,7 @@ bevy_render_macros = { path = "macros", version = "0.14.0-dev" }
bevy_time = { path = "../bevy_time", version = "0.14.0-dev" }
bevy_transform = { path = "../bevy_transform", version = "0.14.0-dev" }
bevy_window = { path = "../bevy_window", version = "0.14.0-dev" }
bevy_winit = { path = "../bevy_winit", optional = true, version = "0.14.0-dev" }
bevy_utils = { path = "../bevy_utils", version = "0.14.0-dev" }
bevy_tasks = { path = "../bevy_tasks", version = "0.14.0-dev" }

Expand Down
16 changes: 16 additions & 0 deletions crates/bevy_render/src/view/window/mod.rs
Expand Up @@ -22,6 +22,8 @@ use wgpu::{
};

pub mod screenshot;
#[cfg(feature = "bevy_winit")]
mod winit;

use screenshot::{
ScreenshotManager, ScreenshotPlugin, ScreenshotPreparedState, ScreenshotToScreenPipeline,
Expand All @@ -34,6 +36,20 @@ pub struct WindowRenderPlugin;
impl Plugin for WindowRenderPlugin {
fn build(&self, app: &mut App) {
app.add_plugins(ScreenshotPlugin);
#[cfg(all(
feature = "bevy_winit",
feature = "multi-threaded",
not(target_arch = "wasm32")
))]
{
// TODO: This is added despite not checking if PipelinedRenderingPlugin is added or not,
// but we can't check right now (since it's added after this plugin)
if app.is_plugin_added::<bevy_winit::WinitPlugin>() {
app.add_plugins(winit::WinitWindowRenderPlugin);
} else {
bevy_utils::tracing::warn!("Winit feature was enabled but couldn't detect WinitPlugin. Are you sure you loaded this after WinitPlugin?");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This potentially hard-couples bevy_render and bevy_winit. I'm not sure if we want to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to alternatives, but i don't see a way around it if we want to properly fix it. There is always the option of using a hack (delaying the winit window removal in app world).

Copy link
Contributor

@Friz64 Friz64 Apr 15, 2024

Choose a reason for hiding this comment

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

@s-puig Take a look at this: Friz64@0561754

It also works as a fix, and avoids the hard-coupling to bevy_winit. I'm not convinced that dynamic dispatch is the most elegant solution here.


if let Ok(render_app) = app.get_sub_app_mut(RenderApp) {
render_app
Expand Down
57 changes: 57 additions & 0 deletions crates/bevy_render/src/view/window/winit.rs
@@ -0,0 +1,57 @@
//! Everything should be kept private since it's unlikely anyone needs to use this directly.
//! Instead, abstractions should be used like [`ExtractedWindow`](crate::view::window::ExtractedWindow).

use crate::{Extract, ExtractSchedule, RenderApp};
use bevy_app::{App, Plugin};
use bevy_ecs::entity::{Entity, EntityHashMap};
use bevy_ecs::system::{NonSend, NonSendMut, Resource};
use bevy_winit::winit;
use std::sync::Arc;

/// This [`Plugin`] extracts [`WinitWindows`](bevy_winit::WinitWindows) into render world.
/// This is needed to avoid crashes *when using pipelined rendering* since the winit window can be
/// modified or removed from app world.
pub struct WinitWindowRenderPlugin;

impl Plugin for WinitWindowRenderPlugin {
fn build(&self, app: &mut App) {
if let Ok(render_app) = app.get_sub_app_mut(RenderApp) {
render_app
.init_non_send_resource::<ExtractedWinitWindows>()
.add_systems(ExtractSchedule, extract_winit_windows);
}
}
}

#[allow(dead_code)]
struct ExtractedWinitWindow {
entity: Entity,
window: Arc<winit::window::Window>,
window_id: winit::window::WindowId,
}

#[derive(Resource, Default)]
struct ExtractedWinitWindows {
windows: EntityHashMap<ExtractedWinitWindow>,
}

fn extract_winit_windows(
mut extracted_windows: NonSendMut<ExtractedWinitWindows>,
windows: Extract<NonSend<bevy_winit::WinitWindows>>,
) {
extracted_windows.windows.clear();
for (&window_id, window) in windows.windows.iter() {
let entity = windows
.winit_to_entity
.get(&window_id)
.expect("Window has no entity mapped. This should be impossible.");
extracted_windows.windows.insert(
*entity,
ExtractedWinitWindow {
window_id,
window: window.clone(),
entity: *entity,
},
);
}
}
3 changes: 2 additions & 1 deletion crates/bevy_winit/src/lib.rs
Expand Up @@ -24,6 +24,7 @@ use bevy_a11y::AccessibilityRequested;
use bevy_utils::Instant;
pub use system::create_windows;
use system::{changed_windows, despawn_windows, CachedWindow};
pub use winit;
use winit::dpi::{LogicalSize, PhysicalSize};
pub use winit_config::*;
pub use winit_event::*;
Expand Down Expand Up @@ -447,7 +448,7 @@ fn handle_winit_event(
// the engine.
if let Some(adapter) = access_kit_adapters.get(&window) {
if let Some(winit_window) = winit_windows.get_window(window) {
adapter.process_event(winit_window, &event);
adapter.process_event(&winit_window, &event);
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_winit/src/system.rs
Expand Up @@ -200,7 +200,7 @@ pub(crate) fn changed_windows(
}

if window.cursor.grab_mode != cache.window.cursor.grab_mode {
crate::winit_windows::attempt_grab(winit_window, window.cursor.grab_mode);
crate::winit_windows::attempt_grab(&winit_window, window.cursor.grab_mode);
}

if window.cursor.visible != cache.window.cursor.visible {
Expand Down
16 changes: 9 additions & 7 deletions crates/bevy_winit/src/winit_windows.rs
Expand Up @@ -4,6 +4,7 @@ use bevy_a11y::{
AccessibilityRequested,
};
use bevy_ecs::entity::Entity;
use std::sync::Arc;

use bevy_ecs::entity::EntityHashMap;
use bevy_utils::{tracing::warn, HashMap};
Expand All @@ -24,7 +25,7 @@ use crate::{
#[derive(Debug, Default)]
pub struct WinitWindows {
/// Stores [`winit`] windows by window identifier.
pub windows: HashMap<winit::window::WindowId, winit::window::Window>,
pub windows: HashMap<winit::window::WindowId, Arc<winit::window::Window>>,
/// Maps entities to `winit` window identifiers.
pub entity_to_winit: EntityHashMap<winit::window::WindowId>,
/// Maps `winit` window identifiers to entities.
Expand All @@ -45,7 +46,7 @@ impl WinitWindows {
adapters: &mut AccessKitAdapters,
handlers: &mut WinitActionHandlers,
accessibility_requested: &AccessibilityRequested,
) -> &winit::window::Window {
) -> Arc<winit::window::Window> {
let mut winit_window_builder = winit::window::WindowBuilder::new();

// Due to a UIA limitation, winit windows need to be invisible for the
Expand Down Expand Up @@ -258,15 +259,16 @@ impl WinitWindows {

self.windows
.entry(winit_window.id())
.insert(winit_window)
.into_mut()
.insert(Arc::new(winit_window))
.get()
.clone()
}

/// Get the winit window that is associated with our entity.
pub fn get_window(&self, entity: Entity) -> Option<&winit::window::Window> {
pub fn get_window(&self, entity: Entity) -> Option<Arc<winit::window::Window>> {
self.entity_to_winit
.get(&entity)
.and_then(|winit_id| self.windows.get(winit_id))
.and_then(|winit_id| self.windows.get(winit_id).cloned())
}

/// Get the entity associated with the winit window id.
Expand All @@ -279,7 +281,7 @@ impl WinitWindows {
/// Remove a window from winit.
///
/// This should mostly just be called when the window is closing.
pub fn remove_window(&mut self, entity: Entity) -> Option<winit::window::Window> {
pub fn remove_window(&mut self, entity: Entity) -> Option<Arc<winit::window::Window>> {
let winit_id = self.entity_to_winit.remove(&entity)?;
// Don't remove from `winit_to_window_id` so we know the window used to exist.
self.windows.remove(&winit_id)
Expand Down