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

Separate Buffer/Encoder for RenderPass Encoders #1300

Merged
merged 3 commits into from
Jun 11, 2017

Conversation

JohnColanduoni
Copy link
Contributor

@JohnColanduoni JohnColanduoni commented Jun 9, 2017

To avoid needing to include traits with lifetime parameters in
the GraphicsCommandBuffer trait, we split the RenderPass*Encoder trait
into RenderPass*Buffer (a backend-implemented trait with no lifetime
parameters) and a struct RenderPass*Encoder containing the necessary
lifetime parameters and associated references.

These structs proxy all their calls to the Buffer trait implementations
by passing a reference to the whole struct that includes the buffer as
a member, as well as the lifetime-bound references. As a result the
backend implementations have access to all the data they had previously
without having to resort to lifetime parameters on the trait or
unsafety.

To avoid needing to include traits with lifetime parameters in
the GraphicsCommandBuffer trait, we split the RenderPass*Encoder trait
into RenderPass*Buffer (a backend-implemented trait with no lifetime
parameters) and a struct RenderPass*Encoder containing the necessary
lifetime parameters and associated references.

These structs proxy all their calls to the Buffer trait implementations
by passing a reference to the whole struct that includes the buffer as
a member, as well as the lifetime-bound references. As a result the
backend implementations have access to all the data they had previously
without having to resort to lifetime parameters on the trait or
unsafety.
@JohnColanduoni
Copy link
Contributor Author

I'll spin up Windows and fix the DX12 backend later tonight, but the Metal and Vulkan backends worked when I tested them.

@kvark kvark self-requested a review June 9, 2017 13:09
@msiglreith msiglreith self-requested a review June 9, 2017 15:36
Copy link
Contributor

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

An improvement over the current implementation 👍

nitpick: I tend to mark functions which shouldn't be called by users additionally with unsafe (along with #[doc(hidden)]). It's a personal preference, I'm not sure how others see it.

@@ -405,8 +405,7 @@ fn main() {
cmd_buffer.bind_graphics_descriptor_sets(&pipeline_layout, 0, &[&set0[0], &set1[0]]);

{
let mut encoder = back::RenderPassInlineEncoder::begin(
&mut cmd_buffer,
let mut encoder = cmd_buffer.begin_render_pass_inline(
Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Much better.

@msiglreith
Copy link
Contributor

For dx12, our current goal should be that it compiles, trianglell won't run anyway at the current state iirc.
Where did appveyor go again?

@JohnColanduoni
Copy link
Contributor Author

On further reflection I think we might actually want to make the RenderPassBuffer traits hidden in their entirety. They never show up in any capacity unless you're writing a backend; all users see is a GraphicsCommandBuffer as a generic parameter for the RenderPassEncoder struct. The duplication of the method names could be quite confusing when reading the documentation.

@kvark
Copy link
Member

kvark commented Jun 9, 2017

So we'll have multiple visibility rings, logically:

  • inner ring - private to gfx_core
  • backend implementors ring
  • users ring, including gfx_render

@JohnColanduoni
Copy link
Contributor Author

The build is only failing on nightly, from an out of memory error when compiling dxguid-sys (which appears to a known bug in rustc).

@@ -20,6 +20,7 @@ use winapi::*;

use core::{self, command, memory, pso, state, target, IndexType, VertexCount, VertexOffset};
use core::buffer::IndexBufferView;
use core::command::{RenderPassInlineEncoder, RenderPassSecondaryEncoder, Encoder};
Copy link
Member

Choose a reason for hiding this comment

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

nit: command is already included, so there should be either just command or all the types we use from it for consistency

@kvark
Copy link
Member

kvark commented Jun 11, 2017

Thank you!
@homu r+

@homu
Copy link
Contributor

homu commented Jun 11, 2017

📌 Commit d78585d has been approved by kvark

homu added a commit that referenced this pull request Jun 11, 2017
Separate Buffer/Encoder for RenderPass Encoders

To avoid needing to include traits with lifetime parameters in
the GraphicsCommandBuffer trait, we split the RenderPass\*Encoder trait
into RenderPass\*Buffer (a backend-implemented trait with no lifetime
parameters) and a struct RenderPass*Encoder containing the necessary
lifetime parameters and associated references.

These structs proxy all their calls to the Buffer trait implementations
by passing a reference to the whole struct that includes the buffer as
a member, as well as the lifetime-bound references. As a result the
backend implementations have access to all the data they had previously
without having to resort to lifetime parameters on the trait or
unsafety.
@homu
Copy link
Contributor

homu commented Jun 11, 2017

⌛ Testing commit d78585d with merge 72c0e10...

@homu
Copy link
Contributor

homu commented Jun 11, 2017

☀️ Test successful - status

@homu homu merged commit d78585d into gfx-rs:master Jun 11, 2017
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.

4 participants