-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Headless renderer example has been added #13006
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
Headless renderer example has been added #13006
Conversation
|
@alice-i-cecile May I ask you for a review? |
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.
Just a quick review of the first couple of things I saw, this isn't a comprehensive review.
The code seems to do a lot of things and I'd prefer if it could be streamlined a bit to be easier to follow, but it's hard to know if there are parts of it that aren't as important because it lacks documentation of what all the pieces are.
782ada3 to
40bb6d8
Compare
e648765 to
4960008
Compare
4960008 to
d85d6a7
Compare
|
@IceSentry , may I ask you for a rereview? |
Co-authored-by: BD103 <[email protected]>
|
could you add objects to the scene, so that it doesn't render just a black picture? the example panics for me: Is it possible to not surface that panic? |
examples/app/headless_renderer.rs
Outdated
| let number = rng.gen::<u128>(); | ||
| let image_path = images_dir.join(format!("{number:032X}.png")); |
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.
Could you use an incrementing number instead of a random value?
replace println to info Co-authored-by: François Mockers <[email protected]>
The reason of this panic is attempt to send image data to nonexistent receiver, because MainWorld destroyed while RenderWorld still renders. For such case I left comment |
11edefd to
946ae14
Compare
|
@mockersf May I ask you for a rereview? |
tychedelia
left a comment
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.
I agree with the comment about the nested modules being unnecessary for an example but otherwise lgtm.
examples/app/headless_renderer.rs
Outdated
| let mut cpu_image = Image { | ||
| texture_descriptor: TextureDescriptor { | ||
| label: None, | ||
| size, | ||
| dimension: TextureDimension::D2, | ||
| format: TextureFormat::Rgba8UnormSrgb, | ||
| mip_level_count: 1, | ||
| sample_count: 1, | ||
| usage: TextureUsages::COPY_DST | TextureUsages::TEXTURE_BINDING, | ||
| view_formats: &[], | ||
| }, | ||
| ..Default::default() | ||
| }; | ||
| cpu_image.resize(size); |
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.
This can be simplified a little with this:
| let mut cpu_image = Image { | |
| texture_descriptor: TextureDescriptor { | |
| label: None, | |
| size, | |
| dimension: TextureDimension::D2, | |
| format: TextureFormat::Rgba8UnormSrgb, | |
| mip_level_count: 1, | |
| sample_count: 1, | |
| usage: TextureUsages::COPY_DST | TextureUsages::TEXTURE_BINDING, | |
| view_formats: &[], | |
| }, | |
| ..Default::default() | |
| }; | |
| cpu_image.resize(size); | |
| let cpu_image = Image::new_fill( | |
| size, | |
| TextureDimension::D2, | |
| &[0; 4], | |
| TextureFormat::bevy_default(), | |
| RenderAssetUsages::default(), | |
| ); |
examples/app/headless_renderer.rs
Outdated
| let mut render_target_image = Image { | ||
| texture_descriptor: TextureDescriptor { | ||
| label: None, | ||
| size, | ||
| dimension: TextureDimension::D2, | ||
| format: TextureFormat::Rgba8UnormSrgb, | ||
| mip_level_count: 1, | ||
| sample_count: 1, | ||
| usage: TextureUsages::COPY_SRC | ||
| | TextureUsages::COPY_DST | ||
| | TextureUsages::TEXTURE_BINDING | ||
| | TextureUsages::RENDER_ATTACHMENT, | ||
| view_formats: &[], | ||
| }, | ||
| ..Default::default() | ||
| }; | ||
| render_target_image.resize(size); |
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.
| let mut render_target_image = Image { | |
| texture_descriptor: TextureDescriptor { | |
| label: None, | |
| size, | |
| dimension: TextureDimension::D2, | |
| format: TextureFormat::Rgba8UnormSrgb, | |
| mip_level_count: 1, | |
| sample_count: 1, | |
| usage: TextureUsages::COPY_SRC | |
| | TextureUsages::COPY_DST | |
| | TextureUsages::TEXTURE_BINDING | |
| | TextureUsages::RENDER_ATTACHMENT, | |
| view_formats: &[], | |
| }, | |
| ..Default::default() | |
| }; | |
| render_target_image.resize(size); | |
| let mut render_target_image = Image::new_fill( | |
| size, | |
| TextureDimension::D2, | |
| &[0; 4], | |
| TextureFormat::bevy_default(), | |
| RenderAssetUsages::default(), | |
| ); | |
| render_target_image .texture_descriptor.usage |= TextureUsages::RENDER_ATTACHMENT | TextureUsages::TEXTURE_BINDING; |
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.
I think there's a few things that can be improved to simplify this example a little, but it's probably better to merge it as-is and improve it later.
I haven't tested my suggestions, but I know they should roughly work. There might be some fmt issues.
|
@bugsweeper those suggestions look solid, but once CI is green I'll merge this in either way :) |
examples/app/headless_renderer.rs
Outdated
| while image_path.exists() { | ||
| number += 1; | ||
| image_path = images_dir.join(format!("{number:03}.png")); | ||
| } |
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.
you could keep a Local<u32> as a system parameter instead of iterating on every file. this has the potential to blow up
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.
This example can be run multiple times with single_image option, Local helps only at first run with multiple files
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.
overwriting files from a previous run seems better to me than iterating over every files. additional bonus is that we create less random files on their computer
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.
Searching free filenames has been changed to Local usage
mockersf
left a comment
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.
interaction with the file system should be improved or commented
08c6aff to
b74e238
Compare
|
@mockersf May I ask you for a rereview? |
|
@alice-i-cecile Ci is green now |
## Objective - Makes `headless_renderer` example work instead of exiting without effect. - Guides users who actually just need [`Screenshot`](https://docs.rs/bevy/0.16.1/bevy/render/view/window/screenshot/struct.Screenshot.html) to use that instead. This PR was inspired by my own efforts to do headless rendering, in which the complexity of the `headless_renderer` example was a distraction, and this comment from #12478 (comment) : > The example added in #13006 would benefit from this change: be sure to clean it up when tackling this work :) That “cleanup” was not done, and I thought to do it, but it seems to me that using `Screenshot` (in its current form) in the example would not be correct, because — if I understand correctly — the example is trying to, potentially, capture many *consecutive* frames, whereas `Screenshot` by itself gives no means to capture multiple frames without gaps or duplicates. But perhaps I am wrong (the code is complex and not clearly documented), or perhaps that feature isn’t worth preserving. In that case, let me know and I will revise this PR. ## Solution - Added `exit_condition: bevy::window::ExitCondition::DontExit` - Added a link to `Screenshot` in the crate documentation. ## Testing - Ran the example and confirmed that it now writes an image file and then exits.
Objective
Fixes #11457.
Fixes #22.
Solution
Based on another headless application
Changelog