-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Use safe window handles and wgpu::Instance::create_surface
#13172
base: main
Are you sure you want to change the base?
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for stopping by, we appreciate it! :)
This change looks promising, though I'm less familiar with the windowing code. Do you think it will hurt performance?
impl fmt::Debug for RawHandleWrapper { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> { | ||
f.debug_struct("RawHandleWrapper").finish_non_exhaustive() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be automatically derived by #[derive(Debug)]
, or does it complain that dyn WindowTrait
does not implement Debug
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's needed since dyn WindowTrait
doesn't implement Debug
. (The standard library has an implementation for dyn Any
.) It seems simplest to just implement Debug
for RawHandleWrapper
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good!
I noticed that bevyengine#12978 introduces a reference counted wrapper around windows, but still uses `RawWindowHandle` and `RawDisplayHandle`, with `wgpu::Instance::create_surface_unsafe`. This can be changed easily enough to use `wgpu::Instance::create_surface`, and not use the raw handles anywhere. Apparently we can just cast a `Arc<W>` to an `Arc<dyn Trait>`, so that makes `WindowWrapper` a little simpler too.
This should have no non-trivial impact on performance. It will just add a reference count to the Otherwise things should be basically unchanged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
no it doesn't |
Hopefully a better solution can be found for that behavior on macOS. But if merging that workaround is the best for now, I guess the changes here can be merged later when that is fixed. Come to think of it, I think Another thing that seems a bit odd is that Wgpu in comparison has |
Yeah, these are just remnants from my PR at this point. I think this PR should address this. |
this crashes on android:
|
Hm, so when Not sure why that would be the case, but I'm not that familiar with how this works on Android. |
Objective
I noticed that #12978 introduces a reference counted wrapper around windows, but still uses
RawWindowHandle
andRawDisplayHandle
, withwgpu::Instance::create_surface_unsafe
. This can be changed easily enough to usewgpu::Instance::create_surface
, and not use the raw handles anywhere.Solution
Instead of containing an
Arc<dyn _>
along with raw handles,RawHandleWrapper
now contains just anArc<dyn _>
, andThreadLockedRawWindowHandleWrapper
implementsHasWindowHandle
/HasDisplayHandle
by calling those those methods on the trait object.bevy_render
useswgpu::Instance::create_surface
with theThreadLockedRawWindowHandleWrapper
. So this increments the reference count, but guarantees the window will outlive thewgpu::Instance
.Apparently we can just cast a
Arc<W>
to anArc<dyn Trait>
, so that makesWindowWrapper
a little simpler too.Testing
I'm not that familiar with Bevy, but examples seem to work, and this shouldn't break anything.
Migration Guide
RawHandleWrapper
no longer haswindow_handle
anddisplay_handle
fields. Instead, theget_handle()
method has to be used.