Using GraphicsPipelineAbstract for ShaderCache makes vertex_buffer break

[ghost]

Hello!

In regards to: #676

I tried to refactor my rendering pipeline a bit and for that I introduced a ShaderCache. In order to be able to put in the various different types of shaders, I went with GraphicsPipelineAbstract, like so:
(I tried to leave out unnecessary details, as it wouldn't render without a lot more files anyway)

pub struct Cache {
    logger: Logger,
    lookup: HashMap<LoadShader, Id>,
    cache: HashMap<Id, Arc<GraphicsPipelineAbstract + Send + Sync>>,
    next_id: MaxNumberOfShaders,
}

impl Cache {
    pub fn new(logger: Logger) -> Self {
        Self {
            logger,
            lookup: HashMap::new(),
            cache: HashMap::new(),
            next_id: 0,
        }
    }

    pub fn get_handle(&mut self, load_shader: LoadShader, device: Arc<Device>, render_pass: Arc<RenderPassAbstract + Send + Sync>) -> Id {
        match self.lookup.get(&load_shader) {
            Some(id) => {
                debug!(self.logger, "Found shader {:?} in cache!", load_shader);
                *id
            }
            None => {
                debug!(self.logger, "Shader {:?} not in cache! Loading now!", load_shader);
                let pipeline = self.assemble(load_shader, device, render_pass);
                let id = self.next_id;
                self.next_id = id.wrapping_add(1);
                let id = Id(id);
                self.cache.insert(id, pipeline);
                id
            }
        }
    }

    pub fn get_pipeline(&self, id: Id) -> Option<&Arc<GraphicsPipelineAbstract + Send + Sync>> {
        self.cache.get(&id)
    }

    fn assemble(&mut self, load_shader: LoadShader, device: Arc<Device>, render_pass: Arc<RenderPassAbstract + Send + Sync>) -> Arc<GraphicsPipelineAbstract + Send + Sync> {
        // FIXME should be inside match statement but then it goes out of scope too fast -> refactor
        let vertex_shader: Arc<chunk_vs::Shader> = Arc::new(chunk_vs::Shader::load(device.clone()).expect("Couldn't load shader"));
        let fragment_shader: Arc<chunk_fs::Shader> = Arc::new(chunk_fs::Shader::load(device.clone()).expect("Couldn't load shader"));

        let subpass = Subpass::from(render_pass, 0).expect("Failed at subpass creation!");

        let pipeline = GraphicsPipeline::start();
        let pipeline = match load_shader.shader_type {
            ShaderTypes::Chunk => {
                pipeline
                    .vertex_input_single_buffer::<CubeVoxelVertex>() // <--------------------------------------------------------------------------------------------------------------------------------- here I had to parameterize the vertex input
                    .vertex_shader(vertex_shader.main_entry_point(), ())
                    .triangle_list()
                    .viewports_dynamic_scissors_irrelevant(1)
                    .fragment_shader(fragment_shader.main_entry_point(), ())
                    .depth_stencil_simple_depth()
            },
            _ => unimplemented!("Loading of type {:?} unimplemented!", load_shader.shader_type)
        };

       let pipeline = match load_shader.draw_mode {
            DrawMode::Fill => pipeline.polygon_mode_fill(),
            DrawMode::Line => pipeline.polygon_mode_line(),
            DrawMode::Point => pipeline.polygon_mode_point(),
        };

        let pipeline = pipeline
            .render_pass(subpass)
            .build(device.clone())
            .expect(&format!("Failed at pipeline creation for {:?}", load_shader));

        Arc::new(pipeline) as Arc<GraphicsPipelineAbstract + Send + Sync> // <---------------------------------------------------------------------------------------------------------------------------------- I think this is the only way to store different shaders, as a GraphicsPipeline (without Abstract suffix ;)) is heavily parameterized.
    }
}

Before my refactoring attempt the pipeline_id was a pipeline of type pub type ChunkShaderPipeline = GraphicsPipeline<SingleBufferDefinition<CubeVoxelVertex>, Box<PipelineLayoutAbstract + Send + Sync>, Arc<RenderPassAbstract + Send + Sync>>;

My vertex_buffer is of type: Arc<CpuAccessibleBuffer<[CubeVoxelVertex]>>.

And I imagined rendering to work like this:

            let pipeline = shader_cache.get_pipeline(render_data.pipeline_id).expect(&format!("No shader for id {:?}", render_data.pipeline_id));
 let vertex_buffer = render_data.vertex_buffer;

            command_buffer_builder = command_buffer_builder
                .draw_indexed(
                    pipeline.clone(),
                    &DynamicState {
                        line_width: None,
                        viewports: Some(vec![Viewport {
                            origin: [0.0, 0.0],
                            dimensions: [dimensions.0, dimensions.1],
                            depth_range: 0.0..1.0,
                        }]),
                        scissors: None,
                    },
                    vertex_buffer.clone(), // <!------- Compile error resides here!
                    render_data.index_buffer.clone(),
                    set,
                    (),
                ).expect("Failed to fill command buffer with chunk data!");

But since my change I get:

= note: expected type std::vec::Vec<std::sync::Arc<(dyn vulkano::buffer::BufferAccess + std::marker::Sync + std::marker::Send + 'static)>>
found type `std::sync::Arc<vulkano::buffer::CpuAccessibleBuffer<[CubeVoxelVertex]>>

My first assumption was, that this happens because in draw_indexed the VertexSource<V> was different to the vertices: V and this is because I lose type-info when casting as GraphicsPipelineAbstract, so I tried to fiddle with https://docs.rs/vulkano/0.10.0/vulkano/pipeline/trait.GraphicsPipelineAbstract.html#tymethod.inner, but alas this one is not public.

I tried a few more things to no avail and now here I am, bothering you, for which I truly am sorry! Maybe someone can enlighten me, because my mistake is probably obvious and really dumb.

[ghost]

Just for the record, I also tried .into() on my GraphicsPipelineAbstract to no avail (well, another compile error that is:

let pipeline: GraphicsPipeline<SingleBufferDefinition, Box<PipelineLayoutAbstract + Send + Sync>, Arc<RenderPassAbstract + Send + Sync>> = pipeline.into();
| ^^^^ the trait std::convert::From<&std::sync::Arc<dyn vulkano::pipeline::GraphicsPipelineAbstract + std::marker::Sync + std::marker::Send>> is not implemented for vulkano::pipeline::GraphicsPipeline<vulkano::pipeline::vertex::SingleBufferDefinition<terrain::blocky::appearance::CubeVoxelVertex>, std::boxed::Box<dyn vulkano::descriptor::PipelineLayoutAbstract + std::marker::Sync + std::marker::Send>, std::sync::Arc<dyn vulkano::framebuffer::RenderPassAbstract + std::marker::Sync + std::marker::Send>>
|
= note: required because of the requirements on the impl of std::convert::Into<vulkano::pipeline::GraphicsPipeline<vulkano::pipeline::vertex::SingleBufferDefinition<terrain::blocky::appearance::CubeVoxelVertex>, std::boxed::Box<dyn vulkano::descriptor::PipelineLayoutAbstract + std::marker::Sync + std::marker::Send>, std::sync::Arc<dyn vulkano::framebuffer::RenderPassAbstract + std::marker::Sync + std::marker::Send>>> for &std::sync::Arc<dyn vulkano::pipeline::GraphicsPipelineAbstract + std::marker::Sync + std::marker::Send>

[rukai]

You can work around the issue by wrapping your buffer in a vec!()
e.g.

                    vec!(vertex_buffer.clone()), // <!------- Compile error resides here!

Needing to wrap the buffer in a vec is definitely a bug.
I have traced the issue to this TODO:

vulkano/vulkano/src/pipeline/vertex/definition.rs

Line 115 in 7a11120

// TODO: better than a Vec

Very confused as to why it was initially implemented as a vec. From the TODO comment, it looks like it might have been implemented as a vec instead of a struct, as a hacky quick implementation? weird.

Edit: I dont think that specific line is actually the issue, but its certainly related.

[ghost]

@rukai thanks, but sadly this isn't enough, because now I get:

expected type std::vec::Vec<std::sync::Arc<(dyn vulkano::buffer::BufferAccess + std::marker::Sync + std::marker::Send + 'static)>>
found type std::vec::Vec<std::sync::Arc<vulkano::buffer::CpuAccessibleBuffer<[terrain::blocky::appearance::CubeVoxelVertex]>>>

[rukai]

Hmmm, before seeing this issue I hadn't gotten this working before, so I tried again on my projects and this time I could resolve the issue (I had more practice at resolving rust compiler errors I guess)

Maybe take a look at what I did? rukai/incipisphere_renderer@de41a06

[ghost]

@rukai

Thanks a lot, for the inspiration in form of the link and your time!

I checked your example and couldn't find a real difference between your approach and mine.

So I tried again, this time exactly as you answered:
When incorporating your solution before, I assigned the "vectorization", like so:

let vertex_buffer = vec!(render_data.vertex_buffer.clone());

and then did:

 .draw_indexed(
                    pipeline.clone(),
                    &DynamicState {
                        line_width: None,
                        viewports: Some(vec![Viewport {
                            origin: [0.0, 0.0],
                            dimensions: [dimensions.0, dimensions.1],
                            depth_range: 0.0..1.0,
                        }]),
                        scissors: None,
                    },
                    vertex_buffer // <------------ here, not "inlined"
...

which led to said error.

If I do it inline, like suggested by you, it works. I am not sure why I trigger that particular error in case I assign it before, but whatever.

Anyway, thanks a lot for your help and your time! I wish you a wonderful Sunday!

[rukai]

Awesome, no worries.

I'll keep the issue open to track the fact that you need a vec, as that sounds like a bug to me.

[rukai]

Found a relevent issue #709
"GraphicsPipelineAbstract will take an associated type VertexDefinition, which will finally make strong typing possible for vertex sources."
Looks like we're waiting on rust features to make this work nicely.

[Rua]

GraphicsPipelineAbstract no longer exists, so this is also no longer an issue.