Skip to content

/gfx

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

Improved gfx_defines documentation #1151

Merged
merged 3 commits into from Feb 20, 2017

Conversation

@Anttonii
Copy link
Contributor

commented Jan 21, 2017 •
edited

This is a draft. Currently looking for feedback on current information and possible improvements.

Closes #1135
@Bastacyclop

Bastacyclop reviewed Jan 21, 2017
View changes

src/render/src/macros/mod.rs Outdated
///
/// pipeline pipe {
/// vbuf: gfx::VertexBuffer<Vertex> = (),
/// transform: gfx::Global<[[f32; 4]; 4]> = "u_Transform",

This comment has been minimized.

Sign in to view
Copy link
@Bastacyclop

Bastacyclop Jan 21, 2017

Member

maybe add a comment to say that this is for compatibility when constant buffers are not supported ?
@Bastacyclop

Bastacyclop reviewed Jan 21, 2017
View changes

src/render/src/macros/mod.rs Outdated
///
/// # `vertex`
///
/// Defines the vertex format

This comment has been minimized.

Sign in to view
Copy link
@Bastacyclop

Bastacyclop Jan 21, 2017

Member

"a vertex format" ? You can have multiple vertex formats.
@Bastacyclop

Bastacyclop reviewed Jan 21, 2017
View changes

src/render/src/macros/mod.rs Outdated
///
/// # `pipe`
///
/// The `pipeline state object` can consist of:

This comment has been minimized.

Sign in to view
Copy link
@Bastacyclop

Bastacyclop Jan 21, 2017

Member

you could introduce the term of "PSO component"
@Bastacyclop

Bastacyclop reviewed Jan 21, 2017
View changes

src/render/src/macros/mod.rs Outdated
///
/// The `pipeline state object` can consist of:
///
/// - A single vertex buffer object to hold the vertices.

This comment has been minimized.

Sign in to view
Copy link
@Bastacyclop

Bastacyclop Jan 21, 2017

Member

You can have an instance buffer too
@Anttonii

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jan 21, 2017

Thanks for the comments, will push a more polished version in a bit.
@kvark

kvark requested changes Jan 22, 2017
View changes

Copy link
Member

left a comment

Looks good! A few changes to address.
src/render/src/macros/mod.rs Outdated
/// The `pipeline state object` or `pso` can consist of the following
/// `pso` components:
///
/// - A single [vertex buffer](pso/buffer/type.VertexBuffer.html) component to hold the vertices.

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jan 22, 2017

Member

The only "single" components at the moment are: depth/stencil and scissor. Vertex/instance buffers can appear multiple times by design.
src/render/src/macros/mod.rs Outdated
/// - A [scissor](pso/target/struct.Scissor.html) rectangle value (DX11)
///
/// Structure of a `pipeline state object` can be defined
/// freely but should at the very least consist of one vertex buffer object.

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jan 22, 2017

Member

No, it doesn't have to have a vertex buffer object.
src/render/src/macros/mod.rs Outdated
/// Defines a structure for shader constant data. This constant data
/// is then appended into a constant buffer in the `pso`. Constant buffers
/// are only supported by DirectX 11 and OpenGL backend uses it's own
/// Uniform Buffer Object which requires OpenGL 3.1.

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jan 22, 2017

Member

It sounds like GL doesn't support the constant buffers. It does, just names them as UBOs. You may just say that constant buffers are supported on D3D11 and GL3.
src/render/src/macros/mod.rs Outdated
/// - Single or multiple [constant buffer](pso/buffer/struct.ConstantBuffer.html) components. (DX11 and OpenGL3)
/// - Single or multiple [global buffer](pos/buffer/struct.Global.html) components.
/// - Single or multiple [samplers](pos/resource/struct.Sampler.html)
/// such as [texture samplers](pso/resource/struct.TextureSampler.html].

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jan 22, 2017

Member

A TextureSampler component is not an example of a sampler, it's a helper that is basically "ShaderResource" + "Sampler", which is useful for OpenGL where each texture comes with a sampler by default.
@Anttonii
Fixed incorrect information
Loading status checks…
467582d
@kvark

kvark requested changes Jan 23, 2017
View changes

Copy link
Member

left a comment

Thanks for the fixes!
There is just a few more to go.
And please squash the commits when ready.
src/render/src/macros/mod.rs Outdated
/// freely but should at the very least consist of one vertex buffer object.
/// Structure of a `pipeline state object` can be defined freely.
///
/// It should be noted however, that you can have multiple objects of everything but

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jan 23, 2017

Member

I think we should call them "components" instead of "objects" here for consistency.
src/render/src/macros/mod.rs Outdated
/// - A [scissor](pso/target/struct.Scissor.html) rectangle value (DX11)
///
/// Structure of a `pipeline state object` can be defined
/// freely but should at the very least consist of one vertex buffer object.
/// Structure of a `pipeline state object` can be defined freely.

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Jan 23, 2017

Member

Let's add a sentence here saying that a definition may be used for multiple PSO objects, which potentially receive different initialization data (like the blend modes).
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jan 30, 2017

@Anttonii are you going to address the latest review? If not, we can take it from here.
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 20, 2017

Ok, I'm going to merge this since the author is not responding.
Future fixes to documentation are needed anyway.
@homu r+
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 20, 2017

📌 Commit 467582d has been approved by kvark
@kvark

kvark approved these changes Feb 20, 2017
View changes

@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 20, 2017

⌛️ Testing commit 467582d with merge 8684e8c...

homu added a commit that referenced this pull request Feb 20, 2017

@homu
Auto merge of #1151 - Anttonii:master, r=kvark 
Improved gfx_defines documentation

This is a draft. Currently looking for feedback on current information and possible improvements.

Closes #1135
Loading status checks…
8684e8c
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 20, 2017

☀️ Test successful - status

@homu homu merged commit 467582d into gfx-rs:master Feb 20, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bastacyclop Bastacyclop

@kvark kvark

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@Anttonii @kvark @homu @Bastacyclop
You can’t perform that action at this time.