Gfx: Proposal: Remove Slice

[nical]

Slice is my least favorite thing in gfx's API:

• "slice" is a very central term in rust, overloading it with a different meaning is confusing (especially since gfx uses real slices in various places).
• Slice contains a bag if information that is useful for the draw call submission but doesn't make sense when creating the vbo/ibo pairs. for example there is no reason for create_something_buffer_blah to return something that has the instance count in it (and because it doesn't makes sense I didn't look for it there, and then I discovered gfx could do instanced draw calls by whining about it not existing on irc).
• Slice makes it hard to discover how to use a index buffers. I believe that people using APIs such as gfx know what index buffers are and and want to use them explicitly. right now they have to figure out that they are hidden inside this Slice thing. No other graphics API hide index buffers in a concept like Slice, so even if it did a good job of simplifying the mental model of rendering geometry (I don't think it does), it would be at the cost of learning something new instead of manipulating what everyone is already familiar with.
• Index buffers is different from other buffers: it's the only buffer that can be either a buffer or no buffers (the magical IndexBuffer::Auto variant). This is inconsistent and confusing. Every other resource is cannot be "null", which encourages you to assume the same for IndexBuffer. I am mentioning my gripe against IndexBuffer being different because it looks like this comes from how it is stored in Slice, so I blame Slice. I think that it is ok for IndexBuffer to be an enum with variant for u16 and u32 indices, (because people can use the API to its full extent without knowing that), but it is not ok to have this Auto variant (a None variant would be bad, Auto is actually even more magical in a bad way). It may look like I am getting too excited about little things, but consistent APIs are a lot easier to learn

So, What I propose is to remove Slice, and add:

pub struct VertexRange(pub u32, pub u32);
pub struct IndexRange(pub u32, pub u32);

// add some methods in VertexRange and IndexRange to make them nice to use,
// like getting sub-ranges, etc.

pub enum DrawType<R> {
    VertexRange { range: VertexRange },
    Indexed { ibo: IndexBuffer<R>, range: IndexRange, base_vertex: VertexCount },
}

// in Encoder:
fn draw<D: PipelineData<R>>(
    &mut self, 
    draw_type: DrawType<R>,
    instances: InstanceCount, 
    pipeline: &PipelineState<R, D::Meta>, 
    user_data: &D
)

Additionally, remove Factory::create_vertex_buffer_with_slice and let people use Factory::create_vertex_buffer and Factory::create_index_buffer separately. And also remove the IndexBuffer::Auto variant.

Replace DrawType with some other name that you like, as long as it reflects that it is something used when submitting draw calls, or split Encoder::draw into Encoder::draw and Encoder::draw_indexed.

If there is no outstanding objection, I'll put up a PR to experiment with this when I have some time to spare (maybe after finishing the current doc PR).

[Bastacyclop]

I'm in favor of removing Slice, but I would not differentiate VertexRange and IndexRange and do something that looks more like inlining:

fn draw<D: PipelineData<R>>(
    &mut self,
    vertex_range: VertexRange,
    base_vertex: VertexCount,
    instances: Option<InstanceParams>,
    index_buffer: Option<IndexBuffer<R>>,
    pipeline: &PipelineState<R, D::Meta>,
    user_data: &D
)

[nical]

I would not differentiate VertexRange and IndexRange

Actually, do you mean that the parameter vertex_range in draw would mean either vertex or index range depending on index_buffer being Some or None?
That sounds very confusing to me, especially since VertexRange sounds like "a range of things in the VertexBuffer" and I would not think about putting an index range in there. If so, maybe remove VertexRange, and use a good old (and not misleadingly typed) std::ops::Range, and call the parameter range instead of vertex_range?

[Bastacyclop]

Yes there might be confusion between the vertices of the vertex buffer and the virtual vertices resulting of indexing, std::ops::Range sounds good to me.

[msiglreith]

We could also just split drawing with and without an index buffer into two methods (draw and draw_indexed) simplifying the method signatures a bit.
Using std::ops::Range sounds really appealing, especially once RangeArgument becomes stable.

[nical]

FWIW, my personal preference is to clearly separate indexed from non-indexed draw calls, either using an enum as parameter or using separate methods in the encoder.

[kvark]

@nical thanks for filing this very detailed RFC! It's a pleasure to read :)

I agree that separating vertex range from index range makes it cleaner, and I'd love to see us using built-in ranges (ops::Range) more aggressively, like here:

pub enum VertexRange<R> {
    Straight { range: Range<VertexCount> },
    Indexed { ibo: IndexBuffer<R>, range: Range<IndexCount>, base_vertex: VertexCount },
}

[bjadamson]

Hey OP, just wondering if you've had any time to work on this. I'm pretty interested in removing slice as well, and as seeing my other pull-request is wrapping up I'm looking for more things to do.. and slice is super confusing

[kvark]

pinging @nical

[nical]

@bjadamson I started toying with this a bit but I am far from getting it building (Slice is all over the place) and more importantly I haven't had a lot of time to spend on it lately. Don't hesitate to take this. I'll put what I have done in a branch somewhere for you to see in case it helps.

[bjadamson]

I'll put what I have done in a branch somewhere for you to see in case it helps.

Siiick thanks! No promises about doing this, but I'll definitely look through your work once you post it :)

[bjadamson]

I started to dig through this, some notes for myself. It seems more practical to wait until the LL branch gets merged into master before working on this, or work on doing this work ontop of the LL branch.

The LL branch seems to contain fixes/simplifications that I'd be duplicating if working from the master branch.

[kvark]

While we agree that Slice isn't the best, there isn't room for long term plans to change it as pre-ll is deprecated now.