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

WIP: Feature multiview #3323

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Limeth
Copy link

@Limeth Limeth commented Aug 2, 2020

Once finished, Fixes #3303.
Hopefully the API usage is correct. An example demonstrating the Multiview feature would be desirable. Feedback is appreciated.

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends:
  • rustfmt run on changed code

Copy link

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review.New suggestions: 2

src/backend/vulkan/src/device.rs Outdated Show resolved Hide resolved
src/backend/vulkan/src/device.rs Outdated Show resolved Hide resolved
Co-authored-by: monocodus[bot] <49363530+monocodus[bot]@users.noreply.github.com>
Comment on lines +655 to +658
pub struct PhysicalDeviceMultiviewProperties {
pub max_multiview_view_count: u32,
pub max_multiview_instance_index: u32,
}
Copy link
Author

@Limeth Limeth Aug 2, 2020

Choose a reason for hiding this comment

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

Substitute for vk::PhysicalDeviceMultiviewProperties without any pointers that would prevent PhysicalDeviceProperties below from being Sync.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you!
Have a few small concerns about the current state of PR

@@ -590,6 +591,10 @@ impl d::Device<B> for Device {

let dependencies = dependencies
.into_iter()
.collect::<Vec<ID::Item>>();
Copy link
Member

Choose a reason for hiding this comment

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

let's use inplace_or_alloc_array instead

}

pub struct PhysicalDeviceProperties {
properties: vk::PhysicalDeviceProperties,
Copy link
Member

Choose a reason for hiding this comment

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

the properties.properies look a bit awful :)
maybe this field name should be raw or inner?

const MESH_SHADER = 0x0000_0002_0000_0000 << 64;

/// Supports multiview
const MULTIVIEW = 0x0001_0000_0000_0000 << 64;
Copy link
Member

Choose a reason for hiding this comment

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

this should be 0x0000_0004_0000_0000 and so on. No need to leave such a big gap with the mesh shader

let mut multiview = None;

// Add extension infos to the p_next chain
if supports_extension(extensions, *KHR_MULTIVIEW) {
Copy link
Member

Choose a reason for hiding this comment

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

something is not right here. Why are we checking for this extension in two places?

Copy link
Author

Choose a reason for hiding this comment

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

This check is performed only once during the initialization of PhysicalDevices, I have now replaced the same check within PhysicalDevice::features(): https://github.com/gfx-rs/gfx/pull/3323/files#diff-44198a1b71ecb41e695db8b6967cd689R1028


Self {
properties: properties2.properties,
multiview: multiview.map(|multiview| multiview.into()),
Copy link
Member

Choose a reason for hiding this comment

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

nit: could write this as muiltiview.map(PhysicalDeviceMultiviewProperties::from)

Copy link

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review.New suggestions: 6

Comment on lines +593 to +594
let (clear_attachments_mask, result) = inplace_it::inplace_or_alloc_array(dependencies.len(), |uninit_guard_dependencies| {
let dependencies = uninit_guard_dependencies.init_with_iter(dependencies);
Copy link

Choose a reason for hiding this comment

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

Suggested change
let (clear_attachments_mask, result) = inplace_it::inplace_or_alloc_array(dependencies.len(), |uninit_guard_dependencies| {
let dependencies = uninit_guard_dependencies.init_with_iter(dependencies);
let (clear_attachments_mask, result) =
inplace_it::inplace_or_alloc_array(dependencies.len(), |uninit_guard_dependencies| {
let dependencies = uninit_guard_dependencies.init_with_iter(dependencies);

generated by rustfmt

Comment on lines +615 to +616
let (clear_attachments_mask, result) = inplace_it::inplace_or_alloc_array(attachments.len(), |uninit_guard_attachments| {
let attachments = uninit_guard_attachments.init_with_iter(attachments);
Copy link

Choose a reason for hiding this comment

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

Suggested change
let (clear_attachments_mask, result) = inplace_it::inplace_or_alloc_array(attachments.len(), |uninit_guard_attachments| {
let attachments = uninit_guard_attachments.init_with_iter(attachments);
let (clear_attachments_mask, result) = inplace_it::inplace_or_alloc_array(
attachments.len(),
|uninit_guard_attachments| {
let attachments = uninit_guard_attachments.init_with_iter(attachments);

generated by rustfmt

Comment on lines 648 to 688
.map(|&id| id as u32)
.collect::<Box<[_]>>();
let resolves = subpass.resolves.iter().map(make_ref).collect::<Box<[_]>>();

(colors, depth_stencil, inputs, preserves, resolves, subpass.view_mask)
})
.collect::<Box<[_]>>();

let result = inplace_it::inplace_or_alloc_array(dependencies.len(), |uninit_guard| {
let dependencies =
uninit_guard.init_with_iter(dependencies);
let subpasses = attachment_refs
.iter()
.map(|(colors, depth_stencil, inputs, preserves, resolves, _view_mask)| {
vk::SubpassDescription {
flags: vk::SubpassDescriptionFlags::empty(),
pipeline_bind_point: vk::PipelineBindPoint::GRAPHICS,
input_attachment_count: inputs.len() as u32,
p_input_attachments: inputs.as_ptr(),
color_attachment_count: colors.len() as u32,
p_color_attachments: colors.as_ptr(),
p_resolve_attachments: if resolves.is_empty() {
ptr::null()
} else {
resolves.as_ptr()
},
p_depth_stencil_attachment: match depth_stencil {
Some(ref aref) => aref as *const _,
None => ptr::null(),
},
preserve_attachment_count: preserves.len() as u32,
p_preserve_attachments: preserves.as_ptr(),
}
})
.collect::<Box<[_]>>();

let multiview_enabled = self.shared.features.contains(Features::MULTIVIEW);

let view_masks = if multiview_enabled {
Some(
attachment_refs
.iter()
.map(|(_colors, _depth_stencil, _inputs, _preserves, _resolves, view_mask)| {
*view_mask
Copy link

Choose a reason for hiding this comment

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

Suggested change
.map(|&id| id as u32)
.collect::<Box<[_]>>();
let resolves = subpass.resolves.iter().map(make_ref).collect::<Box<[_]>>();
(colors, depth_stencil, inputs, preserves, resolves, subpass.view_mask)
})
.collect::<Box<[_]>>();
let result = inplace_it::inplace_or_alloc_array(dependencies.len(), |uninit_guard| {
let dependencies =
uninit_guard.init_with_iter(dependencies);
let subpasses = attachment_refs
.iter()
.map(|(colors, depth_stencil, inputs, preserves, resolves, _view_mask)| {
vk::SubpassDescription {
flags: vk::SubpassDescriptionFlags::empty(),
pipeline_bind_point: vk::PipelineBindPoint::GRAPHICS,
input_attachment_count: inputs.len() as u32,
p_input_attachments: inputs.as_ptr(),
color_attachment_count: colors.len() as u32,
p_color_attachments: colors.as_ptr(),
p_resolve_attachments: if resolves.is_empty() {
ptr::null()
} else {
resolves.as_ptr()
},
p_depth_stencil_attachment: match depth_stencil {
Some(ref aref) => aref as *const _,
None => ptr::null(),
},
preserve_attachment_count: preserves.len() as u32,
p_preserve_attachments: preserves.as_ptr(),
}
})
.collect::<Box<[_]>>();
let multiview_enabled = self.shared.features.contains(Features::MULTIVIEW);
let view_masks = if multiview_enabled {
Some(
attachment_refs
.iter()
.map(|(_colors, _depth_stencil, _inputs, _preserves, _resolves, view_mask)| {
*view_mask
.enumerate()
.filter_map(|(i, at)| {
if at.load_op == vk::AttachmentLoadOp::CLEAR
|| at.stencil_load_op == vk::AttachmentLoadOp::CLEAR
{
Some(1 << i as u64)
} else {
None
}

generated by rustfmt

Comment on lines +690 to +694
.collect::<Vec<u32>>()
)
} else {
None
};
Copy link

Choose a reason for hiding this comment

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

Suggested change
.collect::<Vec<u32>>()
)
} else {
None
};
.sum();
let attachment_refs = subpasses
.into_iter()
.map(|subpass| {
let subpass = subpass.borrow();
fn make_ref(
&(id, layout): &pass::AttachmentRef,
) -> vk::AttachmentReference {
vk::AttachmentReference {
attachment: id as _,
layout: conv::map_image_layout(layout),
}
}
let colors =
subpass.colors.iter().map(make_ref).collect::<Box<[_]>>();
let depth_stencil = subpass.depth_stencil.map(make_ref);
let inputs =
subpass.inputs.iter().map(make_ref).collect::<Box<[_]>>();
let preserves = subpass
.preserves
.iter()
.map(|&id| id as u32)
.collect::<Box<[_]>>();
let resolves =
subpass.resolves.iter().map(make_ref).collect::<Box<[_]>>();
(
colors,
depth_stencil,
inputs,
preserves,
resolves,
subpass.view_mask,
)
})
.collect::<Box<[_]>>();

generated by rustfmt

Comment on lines +724 to +725
info_builder = info_builder.push_next(&mut multiview);
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
info_builder = info_builder.push_next(&mut multiview);
}
let multiview_enabled = self.shared.features.contains(Features::MULTIVIEW);
let view_masks = if multiview_enabled {
Some(
attachment_refs
.iter()
.map(
|(
_colors,
_depth_stencil,
_inputs,
_preserves,
_resolves,
view_mask,
)| { *view_mask },
)
.collect::<Vec<u32>>(),
)
} else {
None
};
let view_offsets = if multiview_enabled {
Some(
dependencies
.iter()
.map(|dependency| dependency.borrow().view_offset)
.collect::<Vec<i32>>(),
)
} else {
None
};
let result = inplace_it::inplace_or_alloc_array(
vk_dependencies.len(),
|uninit_guard| {
let vk_dependencies = uninit_guard.init_with_iter(vk_dependencies);
let mut info_builder = vk::RenderPassCreateInfo::builder()
.flags(vk::RenderPassCreateFlags::empty())
.attachments(&attachments)
.subpasses(&subpasses)
.dependencies(&vk_dependencies);
let mut multiview;
if multiview_enabled {
multiview = vk::RenderPassMultiviewCreateInfo::builder()
.view_masks(&view_masks.unwrap())
.view_offsets(&view_offsets.unwrap())
.correlation_masks(correlation_masks.unwrap())
.build();
info_builder = info_builder.push_next(&mut multiview);
}
self.shared
.raw
.create_render_pass(&info_builder.build(), None)
},
);

generated by rustfmt

Comment on lines +727 to +728
self.shared.raw.create_render_pass(&info_builder.build(), None)
});
Copy link

Choose a reason for hiding this comment

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

Suggested change
self.shared.raw.create_render_pass(&info_builder.build(), None)
});
(clear_attachments_mask, result)
},
);

generated by rustfmt

Copy link

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

This is an autogenerated code review, only first 25 of 30 new suggestions are shown

Comment on lines +31 to +33
buffer,
command,
format as f,
Copy link

Choose a reason for hiding this comment

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

Suggested change
buffer,
command,
format as f,
buffer, command, format as f,

generated by rustfmt

Comment on lines +35 to +37
image as i,
memory as m,
pass,
Copy link

Choose a reason for hiding this comment

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

Suggested change
image as i,
memory as m,
pass,
image as i, memory as m, pass,

generated by rustfmt

Comment on lines +80 to +81
levels: 0 .. 1,
layers: 0 .. 1,
Copy link

Choose a reason for hiding this comment

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

Suggested change
levels: 0 .. 1,
layers: 0 .. 1,
levels: 0..1,
layers: 0..1,

generated by rustfmt

Comment on lines +118 to +119
wb.clone().with_title("quad view 1".to_string()).build(&event_loop).unwrap(),
wb.clone().with_title("quad view 2".to_string()).build(&event_loop).unwrap(),
Copy link

Choose a reason for hiding this comment

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

Suggested change
wb.clone().with_title("quad view 1".to_string()).build(&event_loop).unwrap(),
wb.clone().with_title("quad view 2".to_string()).build(&event_loop).unwrap(),
wb.clone()
.with_title("quad view 1".to_string())
.build(&event_loop)
.unwrap(),
wb.clone()
.with_title("quad view 2".to_string())
.build(&event_loop)
.unwrap(),

generated by rustfmt

*control_flow = winit::event_loop::ControlFlow::Wait;

match event {
winit::event::Event::WindowEvent { event, window_id, .. } => match event {
Copy link

Choose a reason for hiding this comment

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

Suggested change
winit::event::Event::WindowEvent { event, window_id, .. } => match event {
winit::event::Event::WindowEvent {
event, window_id, ..
} => match event {

generated by rustfmt

unsafe {
device.create_pipeline_layout(
iter::once(&*set_layout),
&[(pso::ShaderStageFlags::VERTEX, 0 .. 8)],
Copy link

Choose a reason for hiding this comment

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

Suggested change
&[(pso::ShaderStageFlags::VERTEX, 0 .. 8)],
&[(pso::ShaderStageFlags::VERTEX, 0..8)],

generated by rustfmt

Comment on lines +727 to +733
let vertex_buffers = vec![
pso::VertexBufferDesc {
binding: 0,
stride: mem::size_of::<Vertex>() as u32,
rate: VertexInputRate::Vertex,
},
];
Copy link

Choose a reason for hiding this comment

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

Suggested change
let vertex_buffers = vec![
pso::VertexBufferDesc {
binding: 0,
stride: mem::size_of::<Vertex>() as u32,
rate: VertexInputRate::Vertex,
},
];
let vertex_buffers = vec![pso::VertexBufferDesc {
binding: 0,
stride: mem::size_of::<Vertex>() as u32,
rate: VertexInputRate::Vertex,
}];

generated by rustfmt

w: extents[0].width as _,
h: extents[0].height as _,
},
depth: 0.0 .. 1.0,
Copy link

Choose a reason for hiding this comment

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

Suggested change
depth: 0.0 .. 1.0,
depth: 0.0..1.0,

generated by rustfmt

w: extents[1].width as _,
h: extents[1].height as _,
},
depth: 0.0 .. 1.0,
Copy link

Choose a reason for hiding this comment

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

Suggested change
depth: 0.0 .. 1.0,
depth: 0.0..1.0,

generated by rustfmt

}

fn get_window_index(&self, window_id: winit::window::WindowId) -> usize {
if window_id == self.windows[0].id() { 0 } else { 1 }
Copy link

Choose a reason for hiding this comment

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

Suggested change
if window_id == self.windows[0].id() { 0 } else { 1 }
if window_id == self.windows[0].id() {
0
} else {
1
}

generated by rustfmt

@Limeth
Copy link
Author

Limeth commented Aug 3, 2020

The @monocodus bot seems to have been recommending odd style changes, such as incorrect indentation, so I did not apply their suggestions this time.
I have tried creating a Multiview example derived from the quad example, but I got stuck when implementing the synchronization logic, as I do not feel confident in my understanding of it and how it is structured in gfx.

@kvark
Copy link
Member

kvark commented Aug 3, 2020

@Limeth we can deal with monocodus later.
The synchronization model is exactly what Vulkan has.

@pavel-ignatovich
Copy link

@Limeth Hey, I noticed you're having some problem with monocodus. I've tried to run rustfmt locally on a given code to figure out which part of the suggestion is wrong but it seems like it gives the same result as listed in the suggestion. Could you please point out the wrong indentation in the suggestion?

Also please feel free to use our issue tracker to describe what exactly is wrong, so that our team can resolve the issue in a timely manner.

@Limeth
Copy link
Author

Limeth commented Aug 18, 2020

@P-ignatovich Please see the suggested changes I approved previously: 2bfbd4d

@pavel-ignatovich
Copy link

@Limeth Thank you for your answer!
I took a closer look at these changes and found the issues you mentioned. They're caused by incorrect processing of huge formatting fixes by monocodus. We'll see what we can do about it.

For now, I've disabled rustfmt in out default config so users don't get confused with its behavior.

Thank you very much for your report! Hope we'll make it work better soon :)

@kvark kvark force-pushed the master branch 3 times, most recently from 3e1f8b1 to 375af89 Compare August 19, 2020 14:50
@kvark
Copy link
Member

kvark commented Sep 14, 2020

@Limeth are you going to come back to this PR?

@Limeth
Copy link
Author

Limeth commented Oct 12, 2020

I would like to, but I've been hesitant because there's some kind of UB making it crash on the example, and I couldn't figure out what it was caused by.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Multiview (Multi Viewport)
3 participants