Add option to create headless windows#3835
Conversation
alice-i-cecile
added
A-Rendering
| } | ||
| } | ||
|
|
||
| fn try_create_surface(app: &mut App, wgpu_instance: &wgpu::Instance) -> Option<Surface> { |
There was a problem hiding this comment.
This should return a Result, not an Option IMO.
There was a problem hiding this comment.
I can see why that would make sense. Then the caller would know why they didn't get a surface (no window resource for example). On the other hand, we don't actually use the reason for not getting a surface anywhere (yet?).
If this returned an error, currently we'd have to convert it back into an Option to pass it into RequestAdapterOptions
There was a problem hiding this comment.
Hmm, that's fair. Since this isn't pub that's probably fine for now.
| let surface = window_surfaces | ||
| .surfaces | ||
| .entry(window.id) | ||
| .or_insert_with(|| unsafe { |
There was a problem hiding this comment.
Good point. As far as I can tell, it has never had a safety comment. I am also not experienced enough with the codebase to know why this unsafe is OK here (or maybe it even isn't, and we haven't noticed yet).
There was a problem hiding this comment.
Alright. @cart / @superdump if you can quickly explain why this is safe during review that would be appreciated, but it shouldn't block this PR.
There was a problem hiding this comment.
from https://docs.rs/wgpu/latest/wgpu/struct.Instance.html#safety-2:
Raw Window Handle must be a valid object to create a surface upon and must remain valid for the lifetime of the returned surface.
There was a problem hiding this comment.
These safety requirements have always been slightly spooky. That is, imo a reasonable safety comment here is ¯\_(ツ)_/¯ :).
To my knowledge, it's safe, but it's very difficult to prove that. I don't think winit documents that this requirement is met, so any use of this is mostly best effort.
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Excellent! This a very useful functionality, and generally looks very well done. I've left a couple nits.
IMO we can expand the "How to test UI" test in further PRs.
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
@Wcubed I'd like to merge this soon; can you please rebase? |
|
@alice-i-cecile cart didn't like the idea last time I asked him :( |
|
Alright, fair. I'm going to close this out for now then; we can try again once #4530 is in, which does have consensus. |
5 participants
This adds the ability to pass
Noneto aWindow'sraw_window_handlefield.Includes a test to demonstrate how this allows for headless testing of UI related stuff which requires windows. In this case: the positioning of a button.
Closes #3754