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

[Merged by Bors] - WebGL2 support #3039

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/bevy_internal/src/default_plugins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ impl PluginGroup for PipelinedDefaultPlugins {
group.add(bevy_asset::AssetPlugin::default());
group.add(bevy_scene::ScenePlugin::default());

#[cfg(feature = "bevy_winit")]
group.add(bevy_winit::WinitPlugin::default());

#[cfg(feature = "bevy_render2")]
{
group.add(bevy_render2::RenderPlugin::default());
Expand All @@ -137,8 +140,5 @@ impl PluginGroup for PipelinedDefaultPlugins {
#[cfg(feature = "bevy_gltf2")]
group.add(bevy_gltf2::GltfPlugin::default());
}

#[cfg(feature = "bevy_winit")]
group.add(bevy_winit::WinitPlugin::default());
}
}
51 changes: 36 additions & 15 deletions crates/bevy_winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ use winit::dpi::LogicalSize;
target_os = "netbsd",
target_os = "openbsd"
))]
use winit::platform::unix::EventLoopExtUnix;

Copy link
Member

Choose a reason for hiding this comment

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

the cfg before should be removed also

Copy link
Member

Choose a reason for hiding this comment

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

bawhaha well that wasn't smart of me

#[derive(Default)]
pub struct WinitPlugin;

Expand All @@ -43,6 +41,9 @@ impl Plugin for WinitPlugin {
app.init_resource::<WinitWindows>()
.set_runner(winit_runner)
.add_system_to_stage(CoreStage::PostUpdate, change_window.exclusive_system());
let event_loop = EventLoop::new();
handle_initial_window_events(&mut app.world, &event_loop);
app.insert_non_send_resource(event_loop);
}
}

Expand Down Expand Up @@ -207,21 +208,22 @@ where
}

pub fn winit_runner(app: App) {
winit_runner_with(app, EventLoop::new());
winit_runner_with(app);
}

#[cfg(any(
target_os = "linux",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "netbsd",
target_os = "openbsd"
))]
pub fn winit_runner_any_thread(app: App) {
winit_runner_with(app, EventLoop::new_any_thread());
}

pub fn winit_runner_with(mut app: App, mut event_loop: EventLoop<()>) {
// #[cfg(any(
// target_os = "linux",
// target_os = "dragonfly",
// target_os = "freebsd",
// target_os = "netbsd",
// target_os = "openbsd"
// ))]
// pub fn winit_runner_any_thread(app: App) {
// winit_runner_with(app, EventLoop::new_any_thread());
// }

pub fn winit_runner_with(mut app: App) {
let mut event_loop = app.world.remove_non_send::<EventLoop<()>>().unwrap();
let mut create_window_event_reader = ManualEventReader::<CreateWindow>::default();
let mut app_exit_event_reader = ManualEventReader::<AppExit>::default();
app.world.insert_non_send(event_loop.create_proxy());
Expand Down Expand Up @@ -525,3 +527,22 @@ fn handle_create_window_events(
});
}
}

fn handle_initial_window_events(world: &mut World, event_loop: &EventLoop<()>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do this in the winit_runner_with and keep the EventLoop argument of winit_runner_with?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cart ? (it is your code :) But it seems handle_initial_window_events needs to be called as is, because we need to have a window when renderer_plugin is initialized.

Copy link
Member

Choose a reason for hiding this comment

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

Yup sadly its an order of operations thing. We need to init these windows before the RenderPlugin is setup, not before the app starts running.

let world = world.cell();
let mut winit_windows = world.get_resource_mut::<WinitWindows>().unwrap();
let mut windows = world.get_resource_mut::<Windows>().unwrap();
let mut create_window_events = world.get_resource_mut::<Events<CreateWindow>>().unwrap();
let mut window_created_events = world.get_resource_mut::<Events<WindowCreated>>().unwrap();
for create_window_event in create_window_events.drain() {
let window = winit_windows.create_window(
event_loop,
create_window_event.id,
&create_window_event.descriptor,
);
windows.add(window);
window_created_events.send(WindowCreated {
id: create_window_event.id,
});
}
}
3 changes: 3 additions & 0 deletions pipelined/bevy_render2/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ hexasphere = "4.0"
parking_lot = "0.11.0"
crevice = { path = "../../crates/crevice", version = "0.6.0" }

[target.'cfg(target_arch = "wasm32")'.dependencies]
wgpu = { version = "0.11.0", features = ["spirv", "webgl"] }

[features]
png = ["image/png"]
hdr = ["image/hdr"]
Expand Down
41 changes: 33 additions & 8 deletions pipelined/bevy_render2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,40 @@ struct ScratchRenderWorld(World);

impl Plugin for RenderPlugin {
fn build(&self, app: &mut App) {
let (instance, device, queue) =
futures_lite::future::block_on(renderer::initialize_renderer(
wgpu::util::backend_bits_from_env().unwrap_or(Backends::PRIMARY),
&wgpu::RequestAdapterOptions {
power_preference: wgpu::PowerPreference::HighPerformance,
..Default::default()
let default_backend = if cfg!(not(target_arch = "wasm32")) {
Backends::PRIMARY
} else {
Backends::GL
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Wgpu be changed to default to webgl on wasm?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, it would be great to prefer webgpu over webgl if available (probably). I'll try to figure out how to do it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think its worth blocking on this, but I agree that its a "nice to have".

let backends = wgpu::util::backend_bits_from_env().unwrap_or(default_backend);
let instance = wgpu::Instance::new(backends);
let surface = {
let world = app.world.cell();
let windows = world.get_resource_mut::<bevy_window::Windows>().unwrap();
let raw_handle = windows.get_primary().map(|window| unsafe {
let handle = window.raw_window_handle().get_handle();
instance.create_surface(&handle)
});
raw_handle
};
let (device, queue) = futures_lite::future::block_on(renderer::initialize_renderer(
&instance,
&wgpu::RequestAdapterOptions {
power_preference: wgpu::PowerPreference::HighPerformance,
compatible_surface: surface.as_ref(),
..Default::default()
},
&wgpu::DeviceDescriptor {
features: wgpu::Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES,
#[cfg(not(target_arch = "wasm32"))]
limits: wgpu::Limits::default(),
#[cfg(target_arch = "wasm32")]
limits: wgpu::Limits {
..wgpu::Limits::downlevel_webgl2_defaults()
},
&wgpu::DeviceDescriptor::default(),
));
..Default::default()
},
));
app.insert_resource(device.clone())
.insert_resource(queue.clone())
.init_resource::<ScratchRenderWorld>();
Expand Down
10 changes: 4 additions & 6 deletions pipelined/bevy_render2/src/renderer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ pub use render_device::*;
use crate::{render_graph::RenderGraph, view::ExtractedWindows};
use bevy_ecs::prelude::*;
use std::sync::Arc;
use wgpu::{Backends, CommandEncoder, DeviceDescriptor, Instance, Queue, RequestAdapterOptions};
use wgpu::{CommandEncoder, DeviceDescriptor, Instance, Queue, RequestAdapterOptions};

pub fn render_system(world: &mut World) {
world.resource_scope(|world, mut graph: Mut<RenderGraph>| {
Expand Down Expand Up @@ -42,12 +42,10 @@ pub type RenderQueue = Arc<Queue>;
pub type RenderInstance = Instance;

pub async fn initialize_renderer(
backends: Backends,
instance: &Instance,
request_adapter_options: &RequestAdapterOptions<'_>,
device_descriptor: &DeviceDescriptor<'_>,
) -> (RenderInstance, RenderDevice, RenderQueue) {
let instance = wgpu::Instance::new(backends);

) -> (RenderDevice, RenderQueue) {
let adapter = instance
.request_adapter(request_adapter_options)
.await
Expand All @@ -72,7 +70,7 @@ pub async fn initialize_renderer(
.unwrap();
let device = Arc::new(device);
let queue = Arc::new(queue);
(instance, RenderDevice::from(device), queue)
(RenderDevice::from(device), queue)
}

pub struct RenderContext {
Expand Down
2 changes: 1 addition & 1 deletion pipelined/bevy_render2/src/texture/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub trait BevyDefault {

impl BevyDefault for wgpu::TextureFormat {
fn bevy_default() -> Self {
if cfg!(target_os = "android") {
if cfg!(target_os = "android") || cfg!(target_arch = "wasm32") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use rgba everywhere? Are there any advantages to bgra on other platforms?

Copy link
Member

Choose a reason for hiding this comment

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

I remember hearing that a lot of hardware only supports (or prefers) bgra swap chains.

Copy link
Member

Choose a reason for hiding this comment

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

Wgpu used to use bgra as a default in their examples, but it looks like at some point they switched to rgba. @kvark is there any reason to default to bgra at this point?

Copy link

Choose a reason for hiding this comment

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

At this point we can default to RGBA. Originally, we weren't sure if RGBA is really supported everywhere for presentation, as weird as it sounds.

// Bgra8UnormSrgb texture missing on some Android devices
wgpu::TextureFormat::Rgba8UnormSrgb
} else {
Expand Down