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.

Sign up for GitHub

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

Update windowing chapter #17

Closed
wants to merge 7 commits into from
Closed

Update windowing chapter #17

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b0bad89
updated swapchain creation examples in the chapter to reflect source
Nekomansa Apr 25, 2023
8ad20d2
updated examples in Windowing chapter to better reflect current source
Nekomansa Apr 27, 2023
5a43845
additional fix to SwapchainImage type
Nekomansa Apr 27, 2023
9ec9810
updated explaination on retrieving the window
Nekomansa Apr 27, 2023
f81cc59
fixed additional instance of device in place of a command buffer allo…
Nekomansa Apr 27, 2023
59cf9ce
resolved merge conflict
Nekomansa Apr 27, 2023
d9d9f65
replaced PresentInfo with SwapchainPresentInfo
Nekomansa Apr 28, 2023
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
25 changes: 10 additions & 15 deletions src/windowing/event_handling.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ invalid:
```rust
use vulkano::swapchain::{SwapchainCreateInfo, SwapchainCreationError};

Event::RedrawEventsCleared => {
Event::MainEventsCleared => {
if recreate_swapchain {
recreate_swapchain = false;

Expand Down Expand Up @@ -108,7 +108,7 @@ if window_resized || recreate_swapchain {
viewport.clone(),
);
command_buffers = get_command_buffers(
&device,
&command_buffer_allocator,
&queue,
&new_pipeline,
&new_framebuffers,
Expand Down Expand Up @@ -160,14 +160,12 @@ use vulkano::swapchain::PresentInfo;

let execution = sync::now(device.clone())
.join(acquire_future)
.then_execute(queue.clone(), command_buffers[image_i].clone())
.then_execute(queue.clone(),
command_buffers[image_i as usize].clone())
Comment on lines +163 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

fmt

Suggested change
.then_execute(queue.clone(),
command_buffers[image_i as usize].clone())
.then_execute(queue.clone(), command_buffers[image_i as usize].clone())

.unwrap()
.then_swapchain_present(
queue.clone(),
PresentInfo {
index: image_i,
..PresentInfo::swapchain(swapchain.clone())
},
SwapchainPresentInfo::swapchain_image_index(swapchain.clone(), image_i)
)
.then_signal_fence_and_flush();
```
Expand Down Expand Up @@ -257,7 +255,7 @@ resources:
```rust
// wait for the fence related to this image to finish
// normally this would be the oldest fence, that most likely have already finished
if let Some(image_fence) = &fences[image_i] {
if let Some(image_fence) = &fences[image_i as usize] {
image_fence.wait(None).unwrap();
}
```
Expand All @@ -266,7 +264,7 @@ We will join with the future from the previous frame, so that we only need to sy
future doesn't already exist:

```rust
let previous_future = match fences[previous_fence_i].clone() {
let previous_future = match fences[previous_fence_i as usize].clone() {
// Create a NowFuture
None => {
let mut now = sync::now(device.clone());
Expand All @@ -288,22 +286,19 @@ Now that we have the `previous_future`, we can join and create a new one as usua
```rust
let future = previous_future
.join(acquire_future)
.then_execute(queue.clone(), command_buffers[image_i].clone())
.then_execute(queue.clone(), command_buffers[image_i as usize].clone())
.unwrap()
.then_swapchain_present(
queue.clone(),
PresentInfo {
index: image_i,
..PresentInfo::swapchain(swapchain.clone())
},
SwapchainPresentInfo::swapchain_image_index(swapchain.clone(), image_i),
)
.then_signal_fence_and_flush();
```

And then substitute the old (obsolete) fence in the error handling:

```rust
fences[image_i] = match future {
fences[image_i as usize] = match future {
Ok(value) => Some(Arc::new(value)),
Err(FlushError::OutOfDate) => {
recreate_swapchain = true;
Expand Down
21 changes: 20 additions & 1 deletion src/windowing/introduction.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,30 @@ let event_loop = EventLoop::new(); // ignore this for now
let surface = WindowBuilder::new()
.build_vk_surface(&event_loop, instance.clone())
.unwrap();

let window = surface
.object()
.unwrap()
.clone()
.downcast::<Window>()
.unwrap();
Comment on lines +58 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicated since it also appears below. I like that put a separate version of this together with the explanation, so I think it's best if you stick only with the below one and remove this one.

Suggested change
let window = surface
.object()
.unwrap()
.clone()
.downcast::<Window>()
.unwrap();

```

As you can see, we created a new object, called *surface*.

The *surface* is a cross-platform abstraction over the actual window object, that vulkano can use
for rendering. As for the window itself, it can be retrieved by calling `surface.window()`, which
for rendering. As for the window itself, it can be retrieved this way:

```rust
Copy link
Contributor

Choose a reason for hiding this comment

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

Window is not in scope.

Suggested change
```rust
```rust
use winit::window::Window;

let window = surface
.object()
.unwrap()
.clone()
.downcast::<Window>()
.unwrap();
Comment on lines +74 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

fmt

Suggested change
.object()
.unwrap()
.clone()
.downcast::<Window>()
.unwrap();
.object()
.unwrap()
.clone()
.downcast::<Window>()
.unwrap();

```

which
you can use to manipulate and change its default properties.
Comment on lines +80 to 82
Copy link
Contributor

Choose a reason for hiding this comment

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

This continues a sentence from above the code block, which is generally a no-no as it can be a bit hard to read.

More importantly however, ideally users wouldn't use this procedure every time they need to access the window. This is something users seem to be doing and so I would like to make this distinction clearer. I'm not sure what the best phrasing is myself (feel free to comment).

All in all, maybe it's best to remove this entirely.

Suggested change
which
you can use to manipulate and change its default properties.

Copy link
Contributor

@marc0246 marc0246 May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Update: the issue has just been resolved upstream. Therefore there is no further clarification needed, as this way of creating the surface is going to be deprecated in the next release.


After you made the change, running the program should now open a window, then immediately
Expand Down Expand Up @@ -98,6 +116,7 @@ makes our closure set the `control_flow` to `ControlFlow::Exit` which signals to
an exit.

<!-- todo: is this correct? -->

<!-- > **Note**: Since there is nothing to stop it, the window will try to update as quickly as it can,
> likely using all the power it can get from one of your cores.
> We will change that, however, in the incoming chapters. -->
Expand Down
19 changes: 9 additions & 10 deletions src/windowing/other_initialization.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ invalid format errors:
```rust
use vulkano::render_pass::RenderPass;

fn get_render_pass(device: Arc<Device>, swapchain: &Arc<Swapchain<Window>>) -> Arc<RenderPass> {
fn get_render_pass(device: Arc<Device>, swapchain: &Arc<Swapchain>) -> Arc<RenderPass> {
vulkano::single_pass_renderpass!(
device,
attachments: {
Expand Down Expand Up @@ -42,7 +42,7 @@ use vulkano::image::SwapchainImage;
use vulkano::render_pass::{Framebuffer, FramebufferCreateInfo};

fn get_framebuffers(
images: &[Arc<SwapchainImage<Window>>],
images: &[Arc<SwapchainImage>],
render_pass: &Arc<RenderPass>,
) -> Vec<Arc<Framebuffer>> {
images
Expand Down Expand Up @@ -149,7 +149,6 @@ As for the pipeline, let's initialize the viewport with our window dimensions:

```rust
use vulkano::pipeline::graphics::input_assembly::InputAssemblyState;
use vulkano::pipeline::graphics::vertex_input::BuffersDefinition;
use vulkano::pipeline::graphics::viewport::{Viewport, ViewportState};
use vulkano::pipeline::GraphicsPipeline;
use vulkano::render_pass::Subpass;
Expand All @@ -163,7 +162,7 @@ fn get_pipeline(
viewport: Viewport,
) -> Arc<GraphicsPipeline> {
GraphicsPipeline::start()
.vertex_input_state(BuffersDefinition::new().vertex::<Vertex>())
.vertex_input_state(MyVertex::per_vertex())
.vertex_shader(vs.entry_point("main").unwrap(), ())
.input_assembly_state(InputAssemblyState::new())
.viewport_state(ViewportState::viewport_fixed_scissor_irrelevant([viewport]))
Expand All @@ -178,7 +177,7 @@ fn main() {

let mut viewport = Viewport {
origin: [0.0, 0.0],
dimensions: surface.window().inner_size().into(),
dimensions: window.inner_size().into(),
depth_range: 0.0..1.0,
};

Expand Down Expand Up @@ -206,7 +205,7 @@ also have multiple framebuffers, we will have multiple command buffers as well,
framebuffer. Let's put everything nicely into a function:

```rust
use vulkano::buffer::TypedBufferAccess;
use vulkano::buffer::subbuffer::Subbuffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally prefer importing the re-exported one from the buffer module as that's more succinct. Also, the StandardCommandBufferAllcoator used below is not in scope.

Suggested change
use vulkano::buffer::subbuffer::Subbuffer;
use vulkano::buffer::Subbuffer;
use vulkano::command_buffer::allocator::StandardCommandBufferAllocator;

use vulkano::command_buffer::{
AutoCommandBufferBuilder, CommandBufferUsage, PrimaryAutoCommandBuffer, SubpassContents,
RenderPassBeginInfo,
Expand All @@ -215,17 +214,17 @@ use vulkano::command_buffer::{
use vulkano::device::Queue;

fn get_command_buffers(
device: &Arc<Device>,
command_buffer_allocator: &StandardCommandBufferAllocator,
queue: &Arc<Queue>,
pipeline: &Arc<GraphicsPipeline>,
framebuffers: &Vec<Arc<Framebuffer>>,
vertex_buffer: &Arc<CpuAccessibleBuffer<[MyVertex]>>,
vertex_buffer: &Subbuffer<[MyVertex]>,
) -> Vec<Arc<PrimaryAutoCommandBuffer>> {
framebuffers
.iter()
.map(|framebuffer| {
let mut builder = AutoCommandBufferBuilder::primary(
device.clone(),
command_buffer_allocator,
queue.queue_family_index(),
CommandBufferUsage::MultipleSubmit, // don't forget to write the correct buffer usage
)
Expand Down Expand Up @@ -254,7 +253,7 @@ fn get_command_buffers(

// main()
let mut command_buffers = get_command_buffers(
&device,
&command_buffer_allocator,
&queue,
&pipeline,
&framebuffers,
Expand Down
4 changes: 2 additions & 2 deletions src/windowing/swapchain_creation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ Of all of these properties, we only care about some of them, mainly the dimensio
transparency (composite alpha), and the format of the images.

```rust
let dimensions = window().inner_size();
let dimensions = window.inner_size();
let composite_alpha = caps.supported_composite_alpha.iter().next().unwrap();
let image_format = Some(
physical_device
Expand All @@ -221,7 +221,7 @@ Combining everything, we can create the swapchain:
use vulkano::image::ImageUsage;
use vulkano::swapchain::{Swapchain, SwapchainCreateInfo};

let (swapchain, images) = Swapchain::new(
let (mut swapchain, images) = Swapchain::new(
device.clone(),
surface.clone(),
SwapchainCreateInfo {
Expand Down