Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upPipeline State Objects revolution #828
Conversation
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
ghost
commented
Nov 10, 2015
|
Sorry @sectopod for creating a conflicts. |
sectopod
force-pushed the
sectopod:pso
branch
from
1a7a827
to
5e28e70
Nov 10, 2015
This comment has been minimized.
This comment has been minimized.
|
@csherratt np! let batch : MyBatch<gfx_device_gl::Resources, MyShaderParam>This would be highly unfortunate. Perhaps, I'll avoid touching this for now then. |
This comment has been minimized.
This comment has been minimized.
|
I'm going through several iterations internally, and the progress with this PR is slow but steady. If you need to publish a new version for some other changes - please don't wait for me.
pub struct PipelineState<
R: d::Resources,
I: VertexLayout<R>,
P: ShaderParam<R>,
E: PixelLayout<R>
>(d::handle::RawPipelineState<R>, d::PrimitiveType, I, P, E); |
This comment has been minimized.
This comment has been minimized.
|
Why do we even separate vertex attributes from pixel targets from all the other shader data? If we follow #833, we can just define all the shader interface in a single struct like this one: gfx_shader_link!( Shader {
vertex: VertexBuffer<Vertex>, //regular vertex buffer, where `Vertex: VertexFormat`
vertex_inst: VertexBuffer<Instance>, //instanced buffer, the instance rate is specified somewhere inside the `Instance` struct definition
const_locals: ConstantBuffer<Local>, //constant buffer
const_globals: ConstantBuffer<Global>, // another one
tex_diffuse: TextureView<Dim2, Float4>, // 2D texture SRV
tex_normal: TextureView<Dim2, Float3>, // same, but of `Float3` type
sampler_linear: Sampler, // just a sampler, not sure if it would make sense to typify further here
buf_noise: BufferView<Int4>, // structured buffer SRV
buf_frequency: UnorderedView<Dim2, Int>, // 2D UAV buffer of `Int`
pixel_color: TargetView<Dim2, Float4>, // output render target
});Of course, each of the vertex buffers need to have a special struct behind them too (like the current |
This comment has been minimized.
This comment has been minimized.
|
@sectopod great idea! so we are going to include attributes and targets into a struct what used to be Overall, I think you are not just on the right track, but onto something big. Having all these |
This comment has been minimized.
This comment has been minimized.
|
It took me a fair bit of persistence to implement this scheme, and it could work if finished (got several large commits locally, hesitating to push yet). The reason I stopped is because I feel that this design is incomplete. Take the blend state for example: providing it separately in the A logical conclusion for me is to somehow provide the blending, depth state and offset, and stencil state as a part of this shader link structure. This would mean we no longer need to pass Basically, this shader link has 3 different structures underneath:
So, going back to blend/depth/stencil - we are at the crossroads. Here are some extreme routes we can take, or maybe we can figure out a reasonable compromise in-between (if only anyone would be able to follow my essays...).
Any ideas/opinions on how to proceed? Am I even nearly close to an efficient design here? Without further instructions I'll go with extreme data path to see where the rabbit whole leads... |
This comment has been minimized.
This comment has been minimized.
|
@sectopod nice to see steady progress! It's exciting to see how far we can reach with this shader link concept in terms of safety and usability. BTW, I vote to rename it to As for your main question, I believe a reasonable compromise between data and types w.r.t Edit: now that I think about it more, this is not quite a compromise. Having any data per struct field means exposing this initialization struct type. Dunno how to get this right... cc @csherratt @fkaa |
This comment has been minimized.
This comment has been minimized.
|
Ok, I've been thinking about it some more... I still don't know how to get this right, but at least I know how to definitely get this wrong ;) First off, using types only for PSO initialization ( Secondly, generating the PSO init struct in a macro and then expecting the user to instantiate it is another no-go. Everything that user is supposed to create should be either explicitly documented, or declared by the user themselves. One possible research direction that is left after cutting these paths is - expect the user to provide both init and bind parts explicitly. Reminder: the "init" part is the one passed for PSO creation, the "bind" part is the one provided for each draw call. color: gfx::RenderTargetView<R, Float4> | (&str, gfx::BlendState, gfx::ColorMask),This will generate the |
This comment has been minimized.
This comment has been minimized.
|
Another idea. If we don't take the shader variable names into account, the only data that we need to pass for the PSO creation is related to render targets: blend states, color masks, target formats, depth/stencil. This fact makes the targets somewhat different from the rest of our magical struct. Also the fact that there can be only one depth/stencil view, while there can be any repeating number of other types of entries. Perhaps, we should not include the render targets into this struct then? Treating them separately would make the rest of the struct clean enough and reduce the confusion. For the targets, we can use nominal types (tuples) to avoid user-specified names. |
This comment has been minimized.
This comment has been minimized.
|
@kvark hmm, looks like there is no clearly superior solution in sight. Specifying both the bind data and init data types is too verbose to my taste (I'd rather have the some staff magically derived). Moving render target stuff out would complicate the interface...
|
This comment has been minimized.
This comment has been minimized.
|
I believe I solved the main problem with PSO internal type hierarchy and getting it all defined by helper macros. The biggest issue right now is generating the identifier names in those macros. Since I tried a different approach - to create a new internal module with a user-given name and then just use some predefined simple names inside. However, this turns out to be playing very badly with user-given parameters, which the user expects to be visible, while jumping into a sub-module instantly changes the visibility scope... As always, any ideas are welcome. It would be nice to get this right from the start, since it affects the API verbosity and complexity. |
sectopod
force-pushed the
sectopod:pso
branch
from
b19f000
to
5b63b43
Dec 5, 2015
sectopod
added some commits
Nov 5, 2015
This comment has been minimized.
This comment has been minimized.
|
@cmr thanks! you found quite a few important bits to fix. |
This comment has been minimized.
This comment has been minimized.
|
Who gets the honor of pushing the big green button? :) |
This comment has been minimized.
This comment has been minimized.
|
homu! |
This comment has been minimized.
This comment has been minimized.
|
Oh right, I forget we use homu now! |
This comment has been minimized.
This comment has been minimized.
|
I wonder if we |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
homu
added a commit
that referenced
this pull request
Jan 22, 2016
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
habit, nothing personal ;) |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@cmr here comes the problem: if I push a fix for beta/nightly and politely ask homu to retry, will it retry the same commit you specified?.. |
This comment has been minimized.
This comment has been minimized.
|
Not actually sure... |
This comment has been minimized.
This comment has been minimized.
|
Well, it should be fixed now. The error was real, and I'm glad that beta/nightly caught it. |
sectopod
assigned
cmr
Jan 22, 2016
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
homu
added a commit
that referenced
this pull request
Jan 22, 2016
This comment has been minimized.
This comment has been minimized.
|
|
homu
merged commit 3f92272
into
gfx-rs:master
Jan 22, 2016
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Oh wow, this finally merged? Maybe I should try to raise Rust Theft Auto back from the dead and try this out |
This comment has been minimized.
This comment has been minimized.
|
Hooray! |
sectopod commentedNov 9, 2015
Requires gfx-rs/draw_state#34
Fixes #815, fixes #842, fixes #823, fixes #761,
fixes #722, fixes #703, fixes #694, fixes #463Closes #817, closes #640, closes #630, closes #575, closes #571, closes #322, closes #121
Touches #698, #331, #311, #284, #161
Breaking changes:
Batch,Mesh,Streamconcepts are gone, as well as the old shader parameters interface. PSO is the new paradigm, with it's own macro and interfaces.Surface,Plane,Outputare gone, they are now hidden GL implementation details.Commandenum and state caching have also been moved into GL backend.Texturesemantic has changed - now it is just an allocation of some pixel data. For actual rendering, users are required to operate on the strongly-typed target/resource views. The new format specification supports textures, views, and vertex buffers in the uniform way.See a better overview of the change in this comment. The best source of knowledge to this moment is the example code. We realize such this needs to be well documented and are working on it.
TODO (note: this is a vastly incomplete list of what's actually done):
RenderTargetViewandDepthStencilViewShaderResourceViewandUnorderedAccessViewRendererfor drawingtex::Formatattrib::Formatinit()function for windowing backendsCommandBufferimplementation to do the heavy liftingsupport for structured buffers(can be postponed)Resourcessubtypesremove shader programsPlane,Output, andFrameNewTexturetoTextureprimitivefrommesh::SliceOr, basically, rewrite the whole damn thing ;)