-
Notifications
You must be signed in to change notification settings - Fork 547
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
Swapchain resize support #2052
Swapchain resize support #2052
Conversation
@kvark Can you test this pull request on windows with DX12 and on mac with Metal? I suspect that this patch will stop those backends from compiling properly, but I don't have access to those OSes so I can't test. Can you also review this pull request? Also, for some reason, the image stretches with the window with the vulkan backend but clips on the edges in the gl backend. I assume that it's necessary for both back ends to perform identically, however I'm not sure how I'd go onto doing this. To demonstrate what I mean, here is the gl backend (left) and vulkan backend (right) side by side: |
I'll only be back on Tuesday night. In the meantime, our CI will tell you if there are any build issues on d3d12/metal.
… On May 20, 2018, at 11:40, Hal Gentz ***@***.***> wrote:
@kvark Can you test this pull request on windows with DX12 and on mac with Metal? I suspect that this patch will stop those backends from compiling properly, but I don't have access to those OSes so I can't test.
Can you also review this pull request?
Also, for some reason, the image stretches with the window with the vulkan backend but clips on the edges in the gl backend. I assume that it's necessary for both back ends to perform identically, however I'm not sure how I'd go onto doing this.
To demonstrate what I mean, here is the gl backend (left) and vulkan backend (right) side by side:
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@zegentzy AFAICT stretched image is the desired behavior, since the vertices for the image are specified in NDC space. You could check out the GL backend with renderdoc, might be something related to the viewport? |
The issue was that I wasn't updating the viewport in the gl backend. Fixed in next commit. Now the gl backend is not respecting the quad's position and is stretching it to fill the window. Edit: For some reason the vulkan backend makes things too small. This bug, however, is present on master, so it's outside this pull request's domain. I've filled a separate issue here: #2056. |
I just realized that we need separate semaphores for each image in the swapchain. I'll implement this tomorrow. Edit: Implemented it now. |
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 pushing this! I haven't looked to much into this but to be honest I'm not fully convinced about the refactoring of the quad example. Removing the different shader paths is great, but it ended up in 1300 LOC from 600 LOC prior and it's harder to understand at least for me (introduction of states, lots of Option and not really clear where to start when approaching it).
Would be good to have other opinions on this.
Edit: I wouldn't consider #1961 being resolved by this, only partially so far.
I'll look into refactoring and simplifying the code. Also, why would you consider #1961 not being resolved by this? Do you want separate return codes for OUT_OF_DATE and SUBOPTIMAL? |
src/backend/gl/src/window/glutin.rs
Outdated
f::Format::Rgba8Unorm, | ||
f::Format::Bgra8Unorm, | ||
], | ||
(24, 8, true) => vec![f::Format::Rgba8Srgb, f::Format::Bgra8Srgb], |
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 is an arguable rustfmt change
examples/hal/quad/main.rs
Outdated
Vertex { a_Pos: [ 0.5, 0.33 ], a_Uv: [1.0, 1.0] }, | ||
Vertex { a_Pos: [ 0.5,-0.33 ], a_Uv: [1.0, 0.0] }, | ||
#[cfg(any(feature = "vulkan", feature = "dx12", feature = "metal", feature = "gl"))] | ||
mod renderer { |
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 @msiglreith here that the increased complexity of the quad example is not a good thing. The examples need to be straightforward and readable, also the less code the better. Could you make this simpler?
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 wrapped many things in #[cfg]
s to stop unused variable warnings, I can get rid of them if your ok with warnings on some/all builds.
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.
Do you mean the warnings shown when no backends are selected? We should automatically use the empty
backend in this case.
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.
Yes, but then we'd have to put certain functions behind an feature gate, because they require certain functions not present in the empty backend. I've done so in the next commit and I can revert it if you feel it's more complex.
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.
Which functions aren't present in the empty backend? The empty backend doesn't need to actually run sucessfully, just compile. Ideally the quad example will only have a few cfg
s – only to handle the differences in GL window setup vs. all of the other backends
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've over estimated, ends up there is only one: back::Instance::create()
examples/hal/quad/main.rs
Outdated
a_Uv: [f32; 2], | ||
} | ||
#[macro_use] | ||
extern crate glsl_to_spirv_macros; |
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.
why do we need those macros? I'd prefer us to just do the stuff at run time if it makes us depend on less externals
examples/hal/quad/main.rs
Outdated
extern crate image; | ||
extern crate takeable_option; |
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 we get away without using this?
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.
Lots gfx structs need to be moved into a destroy function like destroy_descriptor_pool
when being destroyed. We can't move things out of structs (even in the Drop function) without putting something back in.
Now, if we had some empty default objects we could do std::mem::replace(old, default). Alternatively, we could use Option and plaster the code with unwraps (which I nor @msiglreith liked) or we can use my thin wrapper around Option which has the Deref and DerefMut traits (which do the unwrapping for us).
Of course, there might be other ways, but I can't think of them.
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.
Isn't this only a problem when you try to wrap things inside a struct that lives longer? The old example code just did
fn main() {
let descriptor_pool = device.create_descriptor_pool(...);
// do stuff
device.destroy_descriptor_pool(descriptor_pool);
}
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.
Yes, only when they are in a structure. I want to avoid having one big function doing everything, and if we don't wrap related fields in structures then we might have to pass half a dozen to a dozen fields to everything.
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 want to avoid having one big function doing everything
This is fine for an example, especially if it simplifies destruction.
cc5c433
to
9c5e19a
Compare
Yes, it's only partially resolving it. To some extent this requires also changes to ash as we currently have 'everything is fine' -> Ok, 'else' -> Err. We should handle it a bit better imo. |
examples/hal/quad/main.rs
Outdated
extern crate image; | ||
extern crate takeable_option; |
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.
do we still need that? I thought we had a consensus that laying down everything into a function body is fine for the example
examples/hal/quad/main.rs
Outdated
#[cfg(any(feature = "vulkan", feature = "dx12", feature = "metal", feature = "gl"))] | ||
fn main() { | ||
env_logger::init(); | ||
struct RendererState<B: Backend> { |
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.
ah, ok. That's definitely a bit more complex than we'd like for this example. Maybe for another one, a more general/complex one, we'll need a struct, but this one is supposed to be as simple as possible.
BTW, quads is WIP |
There seems to be quite a lot of changes in this PR now, so it would be good if we could integrate the swapchain resize changes first IMO (maybe as a separate PR?). |
a2cbfc5
to
b861487
Compare
examples/hal/quad/main.rs
Outdated
|
||
device.wait_for_fence(&frame_fence, !0); | ||
} | ||
|
||
#[cfg(feature = "vulkan")] |
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.
Why a vulkan specific path here?
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.
Only vulkan requires recreating the swapchain, the rest only need updating the viewport. It would be a waste of performance if we recreated it in other backends.
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.
We want all the backends to share the same API, so all of them should be recreating the swapchain (even if there is no actual swapchain for a particular backend like OpenGL)
examples/hal/quad/main.rs
Outdated
if recreate_swapchain { | ||
device.wait_idle().unwrap(); | ||
unsafe { ManuallyDrop::drop(&mut viewport) } | ||
#[cfg(feature = "vulkan")] |
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.
Same here (vulkan specific)
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.
See above
examples/hal/quad/main.rs
Outdated
|
||
if recreate_swapchain { | ||
device.wait_idle().unwrap(); | ||
unsafe { ManuallyDrop::drop(&mut viewport) } |
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.
Why do we need this?
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 we need to destroy the old viewport (because it depends on the swapchain) before making a new one. Non-vulkan backends of course don't need this, but I made it manuallydrop anyways to avoid having two separate let statements.
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.
but viewport is just plain data, it doesn't interact with the swapchain in any hidden way. There should be no need to hack around drop
for the plain data
Backbuffer::Framebuffer(fbo) => (Vec::new(), vec![fbo]), | ||
}; | ||
|
||
let pipeline_layout = |
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.
Why recreate the pipeline layout every time?
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.
The render pass needs to be recreated because it depends on the format of the swap chain images which can, however unlikely, change.. The pipeline depends on that renderpass.
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.
Interesting. Aren't we dictating the format of swapchain images? Why/when would it change?
src/hal/src/adapter.rs
Outdated
@@ -50,7 +50,7 @@ pub struct MemoryProperties { | |||
} | |||
|
|||
/// Represents a physical device (such as a GPU) capable of supporting the given backend. | |||
pub trait PhysicalDevice<B: Backend>: Any + Send + Sync { | |||
pub trait PhysicalDevice<B: Backend>: Any + Send + Sync + Clone { |
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.
What's the motivation behind this one?
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.
We need the physical device when updating the surface's dims (for the vulkan backend), sadly adapter.open_with consumes the adapter (and the physical_device attached with it). That's why we clone the physical for use later.
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 don't quite like the clone-ability of things... we need to consider alternatives, e.g. not consume the physical device in the first place
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 is still a problem
src/hal/src/window.rs
Outdated
@@ -110,6 +110,9 @@ pub struct SurfaceCapabilities { | |||
/// A `Surface` abstracts the surface of a native window, which will be presented | |||
/// on the display. | |||
pub trait Surface<B: Backend>: Any + Send + Sync { | |||
/// Update the window's dimensions (for vulkan) |
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.
Looks strange to have some sort of dimension update, especially for a vulkan only path.
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.
We need to inform the surface that the dimensions have changed somehow else the vulkan backend will make some stuff (I think it was framebuffers? I don't remember) at the wrong size. Might actually be necessary for some other backends too, but it works fine without it in the GL backend,
3c73bdd
to
dab9740
Compare
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 looked over the code and got a few concerns, in addition to what's being commented below.
First, there is a ton of formatting changes. We should not mix it with functional changes, there is already quite a bit of code to look at, and formatting changes only confuse more.
Secondly, and most importantly, I don't like the update_dimensions
approach. I believe we can get away without it. Correct me if I'm wrong, but it appears that the function is only needed because Vulkan backend Surface implementation wants to store/cache the width and height?
If true, we need to address this instead of introducing update_dimensions
and Clone
for PhysicalDevice
.
Vulkan Surface can technically keep a hold of the original connection, be it Xlib/Xcb/Wayland/etc and request the actual dimensions from it later, if needed (for swapchain creation).
I understand this is not a trivial fix, sorry for blowing the scope of the PR out of proportion! We shouldn't regress the API if we know there is a better solution, assuming my proposal makes sense to you.
examples/hal/compute/main.rs
Outdated
let mut glsl = String::new(); | ||
File::open("compute/shader/collatz.comp") | ||
.unwrap() | ||
.read_to_string(&mut glsl) |
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's use a shorter variant here: https://doc.rust-lang.org/stable/std/fs/fn.read_to_string.html
examples/hal/compute/main.rs
Outdated
.unwrap() | ||
.bytes() | ||
{ | ||
spirv.push(byte.unwrap()) |
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 shouldn't be as complex. Maybe just unwrap().bytes().collect::<Vec<_>>()
?
examples/hal/Cargo.toml
Outdated
gfx-hal = { path = "../../src/hal", version = "0.1" } | ||
gfx-backend-empty = { path = "../../src/backend/empty", version = "0.1" } |
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.
where is it used?
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.
No where currently, used to be used. Removed.
@@ -25,7 +25,9 @@ env_logger = "0.5" | |||
image = "0.18" | |||
log = "0.4" | |||
winit = "0.13" | |||
glsl-to-spirv = "0.1.4" |
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 reminds me: we have custom glsl-to-spirv support in some of the backends, like Vulkan.
It doesn't make any sense now, since all the backends accept SPIR-V, so any integration with glsl should be done outside of the backends.
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.
If I understand correctly, you want to strip out glsl-to-spirv from the backends? I think that's outside this PR's job.
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.
Yes, hence the part "this reminds me" :) No action expected from this PR
examples/hal/quad/main.rs
Outdated
use hal::format::{AsFormat, ChannelType, Rgba8Srgb as ColorFormat, Swizzle}; | ||
use hal::pass::Subpass; | ||
use hal::pso::{PipelineStage, ShaderStageFlags, Specialization}; | ||
use hal::queue::Submission; | ||
use hal::{buffer, command, format as f, image as i, memory as m, pass, pool, pso}; | ||
use hal::{Backbuffer, DescriptorPool, FrameSync, Primitive, SwapchainConfig}; |
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 prefer the old formatting. Any reason we need to change it here? Or is is just a rustfmt pass?
We don't want to mix formatting changes with functional one...
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 rustfmt pass, reverting.
Backbuffer::Framebuffer(fbo) => (Vec::new(), vec![fbo]), | ||
}; | ||
|
||
let pipeline_layout = |
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.
Interesting. Aren't we dictating the format of swapchain images? Why/when would it change?
src/backend/dx12/src/window.rs
Outdated
@@ -98,10 +103,13 @@ pub struct Swapchain { | |||
pub(crate) frame_queue: VecDeque<usize>, | |||
#[allow(dead_code)] | |||
pub(crate) rtv_heap: n::DescriptorHeap, | |||
// need to associate raw image pointers with the swapchain so they can be properly released | |||
// when the swapchain is destroyed | |||
pub(crate) resources: Vec<*mut d3d12::ID3D12Resource>, |
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.
shouldn't this be using ComPtr
for releasing?
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 inherited this from hriundel's changes. The correct usage is beyond me, you should ask someone with d3d12 experience.
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.
yes should be a ComPtr
src/backend/vulkan/src/device.rs
Outdated
@@ -1461,6 +1462,11 @@ impl d::Device<B> for Device { | |||
// TODO: handle depth stencil | |||
let format = config.color_format; | |||
|
|||
let mut old_swapchain = vk::SwapchainKHR::null(); | |||
if let Some(osc) = provided_old_swapchain { |
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.
nit: let's do let old_swapchain = match provided_old_swapchain {...}
src/hal/src/adapter.rs
Outdated
@@ -50,7 +50,7 @@ pub struct MemoryProperties { | |||
} | |||
|
|||
/// Represents a physical device (such as a GPU) capable of supporting the given backend. | |||
pub trait PhysicalDevice<B: Backend>: Any + Send + Sync { | |||
pub trait PhysicalDevice<B: Backend>: Any + Send + Sync + Clone { |
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 don't quite like the clone-ability of things... we need to consider alternatives, e.g. not consume the physical device in the first place
examples/hal/quad/main.rs
Outdated
|
||
if recreate_swapchain { | ||
device.wait_idle().unwrap(); | ||
unsafe { ManuallyDrop::drop(&mut viewport) } |
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.
but viewport is just plain data, it doesn't interact with the swapchain in any hidden way. There should be no need to hack around drop
for the plain data
The formats supported by the surface can change between the time of creation of the first swapchain and the new one. We should be querying the surface for the info again, fixed. Edit: Physical device must now be cloned to re query the surface for the new format. I'd argue cloning it is just fine, but two alternatives are:
|
b209087
to
dea2c55
Compare
37b03a0
to
860ff33
Compare
examples/hal/quad/main.rs
Outdated
|
||
// TODO: replace with semaphore | ||
device.wait_for_fence(&frame_fence, !0); | ||
|
||
// present frame | ||
swap_chain.present(&mut queue, &[]); | ||
match swap_chain.present(&mut queue_group.queues[0], &[]) { |
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.
if let Err(_) = swap_chain.present(..) {
..
}
examples/hal/a
Outdated
@@ -0,0 +1,45 @@ | |||
diff --git a/examples/hal/quad/main.rs b/examples/hal/quad/main.rs |
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.
something went wrong here
860ff33
to
7a1bb41
Compare
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 the changes so far. The quad example looks fine to me in the current state.
Adjusting the FFI surface creation function is not possible as we also want to support application without winit.
My current intuition on this topic is that the whole issue is based on this kind
function which was added for convenience but might introduce some issue for certain platforms therefore it's probably worth checking the complexity reduction when removing the function along with the (widht, height) caches inside the surface.
cc @kvark
edit: Further arguable API change: passing a physical device reference together with a logical device reference, which was already created from the same physical device.
src/backend/vulkan/src/window.rs
Outdated
&self, | ||
display: *mut c_void, | ||
surface: *mut c_void, | ||
window: &Arc<winit::Window>, |
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.
We can't just require the user to provide a reference to a winit window as this interface is supposed to work with other crates or even C application as well.
src/backend/vulkan/src/window.rs
Outdated
} | ||
} | ||
|
||
impl hal::Surface<Backend> for Surface { | ||
fn kind(&self) -> hal::image::Kind { | ||
hal::image::Kind::D2(self.width, self.height, 1, self.samples) | ||
hal::image::Kind::D2(self.current_extent.unwrap().width, self.current_extent.unwrap().height, 1, self.samples) |
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 feel like most of the effort went into implementing this function but I don't believe we actually need it.
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.
We need to update the extent for swapchain recreation (so the swapchain images are the right size). Caching it for the kind function adds like ~10 lines.
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.
Correct me if I'm wrong why do we need to cache those at all? Users should just call createswapchain with the old swapchain and the new size gathered from the resize event of the native window or winit. For additional safety checks usere can requery the surface capabilties the check the current extent (optional per platform) and check if the extents are inside the boundaries. Updating these are done by the vulkan drivers already.
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 for addressing the comments!
Some new ones, in addition to @msiglreith concerns, are below:
@@ -25,7 +25,9 @@ env_logger = "0.5" | |||
image = "0.18" | |||
log = "0.4" | |||
winit = "0.13" | |||
glsl-to-spirv = "0.1.4" |
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.
Yes, hence the part "this reminds me" :) No action expected from this PR
src/backend/dx11/src/lib.rs
Outdated
@@ -1053,7 +1054,9 @@ impl hal::Device<Backend> for Device { | |||
&self, | |||
surface: &mut Surface, | |||
config: hal::SwapchainConfig, | |||
) -> (Swapchain, hal::Backbuffer<Backend>) { | |||
old_swapchain: Option<Swapchain>, | |||
_physical_device: &PhysicalDevice, |
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 @msiglreith that it doesn't seem right to accept a PhysicalDevice
in a Device
method.
src/backend/vulkan/src/window.rs
Outdated
@@ -16,13 +16,102 @@ use winit; | |||
use conv; | |||
use {VK_ENTRY, Backend, Instance, PhysicalDevice, QueueFamily, RawInstance}; | |||
|
|||
pub trait GetExtentable { | |||
fn get_extent(&self) -> Option<(u32, u32)>; |
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.
any reason this is not hal::window::Extent2D
?
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.
No
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.
heh, I was reading it over email thinking, hmmm... did I ask "can we use Extend2D
?" and you just replied straight "No" 🤣
src/backend/vulkan/src/window.rs
Outdated
#[cfg(all(unix, not(target_os = "android")))] | ||
Xcb(*mut vk::xcb_connection_t, vk::xcb_window_t), | ||
#[cfg(all(unix, not(target_os = "android")))] | ||
Wayland(Arc<GetExtentable>), |
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.
why are these trait objects instead of concrete types like the other variants?
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.
So we can support non-winit.
src/backend/vulkan/src/window.rs
Outdated
pub(crate) width: Size, | ||
pub(crate) height: Size, | ||
pub(crate) current_extent: Option<hal::window::Extent2D>, | ||
connection: Arc<Mutex<Connection>>, |
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.
who is this shared with?
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.
Nobody as far as I know, the mutex is only used by me insure Sync & Send get implemented. Or do you mean the Arc? That's so that we don't have to use lifetimes which can't be used due to lack of type ascription.
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 mean, could this just be connection: Connection
?
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.
No, else we get errors like:
error[E0277]: `*mut *const libc::c_void` cannot be shared between threads safely
--> src/backend/vulkan/src/window.rs:372:6
|
372 | impl hal::Surface<Backend> for Surface {
| ^^^^^^^^^^^^^^^^^^^^^ `*mut *const libc::c_void` cannot be shared between threads safely
|
= help: within `window::Surface`, the trait `std::marker::Sync` is not implemented for `*mut *const libc::c_void`
= note: required because it appears within the type `window::Connection`
= note: required because it appears within the type `window::Surface`
Which would require using lifetimes to solve, which would require type ascription. That's why we need Mutex, however, I just realized that the Arc is unnecessary, so fixed.
src/backend/vulkan/src/window.rs
Outdated
} | ||
} | ||
|
||
impl hal::Surface<Backend> for Surface { | ||
fn kind(&self) -> hal::image::Kind { | ||
hal::image::Kind::D2(self.width, self.height, 1, self.samples) | ||
hal::image::Kind::D2(self.current_extent.unwrap().width, self.current_extent.unwrap().height, 1, self.samples) |
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's make it cleaner by having let ext = self.current_extent.expect("");
src/backend/vulkan/src/window.rs
Outdated
impl Surface { | ||
pub(crate) fn update_extent(&mut self, physical_device: &PhysicalDevice) { | ||
use hal::Surface; | ||
let (caps, _) = self.capabilities_and_formats(physical_device); |
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.
is there a reason we can't generate caps
during initialization and just keep it instead of re-generating here?
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.
Because the extent changes when the window resizes? How else do I query for the new size?
src/hal/src/adapter.rs
Outdated
@@ -50,7 +50,7 @@ pub struct MemoryProperties { | |||
} | |||
|
|||
/// Represents a physical device (such as a GPU) capable of supporting the given backend. | |||
pub trait PhysicalDevice<B: Backend>: Any + Send + Sync { | |||
pub trait PhysicalDevice<B: Backend>: Any + Send + Sync + Clone { |
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 is still a problem
1692803
to
9b77351
Compare
@@ -46,7 +51,13 @@ fn main() { | |||
.open_with::<_, Compute>(1, |_family| true) | |||
.unwrap(); | |||
|
|||
let shader = device.create_shader_module(include_bytes!("shader/collatz.spv")).unwrap(); | |||
let glsl = fs::read_to_string("compute/shader/collatz.comp").unwrap(); | |||
let spirv: Vec<u8> = glsl_to_spirv::compile(&glsl, glsl_to_spirv::ShaderType::Compute) |
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.
nit: no need to specify the Vec
type both here and in collect()
examples/hal/quad/main.rs
Outdated
Extent2D, | ||
) { | ||
let surface_format = surface | ||
.capabilities_and_formats(device.get_physical_device()) |
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's follow the same logic as a Vulkan application:
- pass in the physical device here that is held by the user
- don't consume it on
open
36d16c0
to
0344e21
Compare
Signed-off-by: Hal Gentz <[email protected]>
0344e21
to
cdb5992
Compare
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.
alright, looks like we are ready now!
bors r+
2052: Swapchain resize support r=kvark a=ZeGentzy Continuation of #1856 When complete, this pull request will solve issues #1961, #1780 and #1854 Todo: - [x] Make quad example use resize PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: vulkan and gl (both tested on ArchLinux) Co-authored-by: Hal Gentz <[email protected]>
Continuation of #1856
When complete, this pull request will solve issues #1961, #1780 and #1854
Todo:
PR checklist:
make
succeeds (on *nix)make reftests
succeeds