feature: added and implemented set_viewport

[EriKWDev]

No description provided.

[EriKWDev]

that's what happens when not at the mac for the metal tests xD

[kvark]

Thank you for this contribution!

In an attempt to reduce the redundancy here, do we really need to switch cases on a per-pipeline basis? That seems to be a rather weird case.

[EriKWDev]

Some context here is that this allows me to port our current cascaded shadowmap implementation verbatim which renders the different cascades to a wide texture, (1024*cascades)*1024 requiring separate viewports and pipelines to render the cascades

Our material system has separate pipelines for alpha-discarded geometry and non-alpha discarded, so even within the same shadowmap renderpass we are changing both pipeline and viewport+scissor to achieve this

Not a deal-breaker of course though as there are ways to do the same using parameters in uniforms to the shader. We could squeeze the clip_pos.x based on which cascade we are rendering

Another solution is I could use an array texture and render to the slices using separate texture views and multiple render-passes, but if I remember correctly we at some point measured and viewport- + scissor-changing in a single-pass was faster.

---

Our approach is inspired from The Witness (http://the-witness.net/news/2010/04/graphics-tech-shadow-maps-part-2-save-25-texture-memory-and-possibly-much-more/ https://blog.thomaspoulet.fr/the-witness-frame-part-1/)

[EriKWDev]

Re-measuring the performance win is ofc in place xD But my first goal is just to port all the features from our current renderer to blade and see, so I will keep this in my fork regardless.

Next-up is planar reflection, refraction, water simulation and DDGI irradiance probes! https://idno.se/swap/

[kvark]

That makes sense, thank you for the explanation!
So your loop may look like this:

for material in [opaque_material, transparent_material] {
  encoder.bind_pipeline(material.pipeline);
  for object in objects.filter_by(material) { 
    encoder.set_viewport();
    encoder.draw();
  }
}

I'm just sad to see the code duplication here. But that's not something your PR introduces - it was already the case for the scissor rect. Carefully ignored :)

[kvark]

I'll think about how to avoid this redundancy.
Maybe the pipeline encoder can deref to the base pass.

[EriKWDev]

The code-duplication seeems avoidable

One benefit too that might arise from a restructure would also be that the different kinds of pipelineencoders can perhaps avoid exposing unrelated functions

Right now dispatch and draw/viewport commands are available regardeless of whether it is a render or a compute even though the .with-function could gurantee the kind, but since both return the same type, PipelineEncoder, all functions get exposed

[EriKWDev]

Like, why should I be able to call set_viewport during compute xP

    if let mut cp = encoder.compute("downsample-depth") {
        let mut c = cp.with(&pipelines.depth_downsample);
        c.set_viewport( // ???
            &gpu::Viewport {
                x: 0.0,
                y: 0.0,
                w: 1920.0,
                h: 1080.0,
            },
            0.0..1.0,
        );
    }

If the with must return the same type, maybe make PipelineEncoder<Marker> contain a PhantomData<Marker> to mark it as either compute or render and then the trait implementations could only be for the PipelineEncoder with the correct marker.

impl crate::traits::ComputeEncoderTrait for PipelineEncoder<ComputeMarker> {
 /// ...
}

Alternatively, make the with return unique types but contain the PipelineEncoder and only implement the relevant trait for the relevant type

struct ComputePipelineEncoder {
  inner: PipelineEncoder,
}

struct RenderPipelineEncoder {
  inner: PipelineEncoder,
}

[kvark]

The main purpose of those types is to be explicit about the semantics. It is not really to protect against mistakes - Blade is an unsafe (lean and mean) graphics API after all. The pipeline context is there because it defines the lifetimes of all bindings. This is the important bit, not the fact of whether or not you can set the scissor in there.

[EriKWDev]

Absolutely, these are the reasons I am so happy to read the code of blade and use it xD I do not intend to challenge those philosophies. My intention was to propose an improvement to the usage of the library at no extra runtime cost, but it is perhaps orthogonal to blade:s design in ways I haven't considered yet.

Let me think about it and examine the code more.

[EriKWDev]

After some exlporing in the code, I still think that overall, making the with function return a distinct types for render and compute in all backends seems like an improvement in several ways:

It would make the implementations more consistent (metal has the RenderPipelineContext and ComputePipelineContext), it would allow for having no "overhead" in gles backend whose shared PipelineEncoder currently contains vertex information in compute, and it would finally remove unnecessary API surface in vulkan/gles that doesn't even compile on macos currently (my silly example above setting viewport in compute).

It would not, however, address the redundancy in set_viewport/set_scissor implementations, so there is probably an even better solution.

[kvark]

I prototyped a few things, and I'm converging on the idea that the duplication of set_viewport is not really a problem.
More concretely:

• on the API side, it doesn't need to be duplicate. Can be expressed with a trait inheritance.
• on the implementation side we can do many things to re-use the code, and it's not much code anyway

So, it's totally cool. I may follow-up with a small refactor but overall we are good.

[EriKWDev]

Nice! Yeah adding the function to turn them into the native type made it much less duplicated xP

Have you considered my other point?

After some further coding I found another argument. I wanted to write a utility function to help with the dispatch of my instances but on vulkan/gles the return type is a gpu::PipelineEncoder and on metal instead a gpu::RenderPipelineContext, so had to make a macro :/

It would really be nice to make the return-type of with part of the guaranteed API-surface. or at least have it return a type implementing a public trait or something.

[kvark]

I understand the pain point you faced. I don't think we can properly expose the "blade_graphics::RenderCommandEncoder" for all backends. Main issue is lifetimes. One backed will have no lifetimes, another backend will need 1, another will need 2. The lifetimes are really an implementation detail of the backend, but they screw up our ability to expose it as the same type.

It would really be nice to make the return-type of with part of the guaranteed API-surface. or at least have it return a type implementing a public trait or something.

We do have a public trait - blade_graphics::traits::RenderEncoder. It's not covering everything, though, but we can work on that.

[EriKWDev]

Aah yeah I see exactly now. Yeah the lifetimes in there would propagate to the user no no...

And didn't know drop specialization didn't work xD very surprising

The public trait might be the way to go then

[EriKWDev]

We do have a public trait - blade_graphics::traits::RenderEncoder

the module traits is not public. The mod is not pub mod :/