Skip to content
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

Figure out why polymorphic typed client code is hard #438

Closed
3 tasks
hadronized opened this issue Aug 18, 2020 · 8 comments
Closed
3 tasks

Figure out why polymorphic typed client code is hard #438

hadronized opened this issue Aug 18, 2020 · 8 comments
Labels
enhancement Performance enhancement, documentation, adding tests, etc. hard An hard feature / bug.

Comments

@hadronized
Copy link
Owner

hadronized commented Aug 18, 2020

@hadronized hadronized added enhancement Performance enhancement, documentation, adding tests, etc. hard An hard feature / bug. labels Aug 18, 2020
@hadronized hadronized added this to the Polymorphic typed client milestone Aug 18, 2020
@kpreid
Copy link
Contributor

kpreid commented Oct 31, 2020

I think it might help a lot if the documentation had examples of what code that takes a polymorphic graphics context has to do — e.g. show that if you're using a Tess, you might need to declare

fn f(ctx: C) where
    C: GraphicsContext,
    C::Backend: luminance::backend::tess::Tess<(), (), (), Interleaved>,
    C::Backend: luminance::backend::tess_gate::TessGate<Vertex, (), (), Interleaved>,
    ...

There should also be enough information to know what goes in place of those ()s in less trivial cases.

It would also reduce new user confusion if the backend traits were not named identically to the frontend types — it's of course possible to import them under new names, as the luminance crate code does, but that doesn't change the compiler error messages, and requires more typing per trait imported.

@hadronized
Copy link
Owner Author

Yeah, the thing is that I’m not sure people should be using these. It might be the sign of a missing piece in the interface, more than lacking documentation. I need to investigate a bit more based on your comments and what I find on my side. Thanks @kpreid.

@kpreid
Copy link
Contributor

kpreid commented Oct 31, 2020

Trying to think of ideas:

  • A brute-force sort of patch to the interface would be to add a trait with many supertraits which names some well-understood bundle of functionality, such as "this backend supports every kind of texture that WebGL 1.0 does", which would specify as supertraits TextureBackend<D, P> for every applicable combination of D and P.

  • In the example I wrote above, having to declare both Tess and TessGate seems odd — surely neither is useful without the other, and they almost share the parameters? And where they don't, why do I need to mention my vertex type in TessGate? I imagine a TessGate (the struct, not the trait) has to be specific to the vertex type to handle setting up the bindings, but when declaring the features I require of the backend, the specific vertex type mattering seems potentially inconvenient. So, can Tess and TessGate be just one backend trait?

@zicklag
Copy link
Contributor

zicklag commented Mar 19, 2021

I haven't looked a lot into this or anything yet, but it seems like the heavy polymorphism defeats Rust analyzer in a number of situations. This is technically a Rust Analyzer problem more than a Luminance problem, but it does end up effecting users of Rust Analyzer so worth knowing about.

@hadronized
Copy link
Owner Author

Ah? What do you mean? So far, I don’t really have issues with it (the proc-macro / macro can but that’s a known issue).

@zicklag
Copy link
Contributor

zicklag commented Mar 21, 2021

It might be specific to my environment somehow, but for some reason none of the closure arguments are picked up by Rust Analyzer when rendering a pipeline. In this picture the green arrows are poperly detected, but none of the types pointed at by red arrows work ( and any of the variables that come from their functions ).

image


You know I may have noticed some other weird behaviour though, to, so I might need to do more investigation. I'll have to uninstall and reinstall Rust analyzer, cargo clean, etc. to make sure.

@kpreid
Copy link
Contributor

kpreid commented Sep 9, 2021

I just finished making my complex application code generic over luminance backends. (I had previously tried and given up, but now I have more experience interpreting rustc's error messages.) Some notes from the experience, which I hope will be useful for API design:

I got the idea of making an "alias" trait that collects all the bounds that a compatible backend needs. This greatly improved the situation over having to repeat the individual bounds (and import the backend traits everywhere), but it's quite long:

    pub trait AicLumBackend:
        Sized
        + FramebufferBackBuffer
        + Pipeline<Dim2>
        + PipelineTexture<Dim2, NormRGBA8UI>
        + PipelineTexture<Dim3, NormRGBA8UI>
        + RenderGate
        + Shader
        + TessGate<(), (), (), Interleaved>
        + TessGate<LumBlockVertex, (), (), Interleaved>
        + TessGate<LumBlockVertex, u32, (), Interleaved>
        + VertexSlice<LumBlockVertex, u32, (), Interleaved, LumBlockVertex>
        + IndexSlice<LumBlockVertex, u32, (), Interleaved>
    where
        (): luminance::backend::depth_slot::DepthSlot<Self, Dim2>,
    {}

Notably, I have to specify TessGate, and sometimes VertexSlice and IndexSlice, for each combination of vertex and index types and layout I use. Perhaps there could be a single trait which combined all three of these and Tess too, if there is not a reason why one trait would be supported but not the others? Also, when working though compiler errors to add needed bounds, it's easy to end up with an even longer list involving traits such as Tess, Texture, and PipelineBase even though those are supertraits implied by the above list of bounds. This might be helped by documentation on the backend traits, such as Tess saying "You probably want to bound the backend by TessGate which implies this”, if it's not possible to combine the traits.

The above is merely verbosity, but there's also an inability-to-abstract problem: anything that sets uniforms has to declare Uniformable bounds for every involved concrete type, and putting those bounds in the above trait doesn't satisfy the compiler (even though it does for DepthSlot), which means lists of bounds like this on any function that calls a function that sets a batch of uniforms, even when the caller doesn't care about any of those specific types:

        f32: Uniformable<Backend>,
        [i32; 3]: Uniformable<Backend>,
        [f32; 3]: Uniformable<Backend>,
        [[f32; 4]; 4]: Uniformable<Backend>,
        TextureBinding<Dim2, NormUnsigned>: Uniformable<Backend>,
        TextureBinding<Dim3, NormUnsigned>: Uniformable<Backend>,

This seems like it could be addressed by switching the types around so that the bound is B: Uniformable<T> instead of T: Uniformable<B>, so they work like the other bounds above.

hadronized added a commit that referenced this issue Oct 10, 2021
The implementor and the type variable are now swapped. Instead of
implementing it as:

```rust
impl Uniformable<TheBackendType> for [f32; 3] { /* … */ }
```

It is now implemented as:

```rust
impl Uniformable<[f32; 3]> for TheBackendType { /* … */ }
```

This allows for better polymorphic code. Next commit should introduce a
type family to be able to manipulate references correctly, and use the
`luminance::shader::types` types, since the current implementors are
wrong and ambiguous.

Relates to #438.
hadronized added a commit that referenced this issue Oct 10, 2021
This commit provides support for existential in `Uniform<T>`, so that
it’s possible to ask for things like `&'a [T; N]`, where `'a` comes from
call-site.

The current implementation is not perfect as it would be normally
implemented with GATs… but I don’t have GATs.

Relates to #438.
hadronized added a commit that referenced this issue Oct 11, 2021
This commit provides support for existential in `Uniform<T>`, so that
it’s possible to ask for things like `&'a [T; N]`, where `'a` comes from
call-site.

The current implementation is not perfect as it would be normally
implemented with GATs… but I don’t have GATs.

Relates to #438.
hadronized added a commit that referenced this issue Oct 11, 2021
This commit provides support for existential in `Uniform<T>`, so that
it’s possible to ask for things like `&'a [T; N]`, where `'a` comes from
call-site.

The current implementation is not perfect as it would be normally
implemented with GATs… but I don’t have GATs.

Relates to #438.
hadronized added a commit that referenced this issue Oct 11, 2021
This commit provides support for existential in `Uniform<T>`, so that
it’s possible to ask for things like `&'a [T; N]`, where `'a` comes from
call-site.

The current implementation is not perfect as it would be normally
implemented with GATs… but I don’t have GATs.

Relates to #438.
hadronized added a commit that referenced this issue Oct 11, 2021
This commit provides support for existential in `Uniform<T>`, so that
it’s possible to ask for things like `&'a [T; N]`, where `'a` comes from
call-site.

The current implementation is not perfect as it would be normally
implemented with GATs… but I don’t have GATs.

Relates to #438.
@hadronized
Copy link
Owner Author

hadronized commented Oct 17, 2021

So after having tried several times to fix that problem, I came up with two main results:

  • Not using luminance-front ends up with annotating lots of types with lots of luminance trait bounds. I wrote a lot of test code, especially in Haskell. If I can’t do it in Haskell, I won’t be able to do it in Rust either.
  • I tried to change the approach by forcing the backends to implement all the same types.

The first point was a failure. The reason for that is because there is no way to infer the constraints in a rank-2 kind of way. Something like:

fn foo<B>() where B: SomeBackendStuff {
  let x: Foo<B, u32> = B::new_foo(); // that would bring FooBackend<u32>
}

Is impossible if SomeBackendStuff doesn’t bring in FooBackend<u32>. So the constraints must be annotated.

This (sad) result made me come to the realization that I could probably define a limited set of types that must be supported, such as in:

trait SomeBackendStuff: FooBackend<u32> + FooBackend<i32> + …

This works « in theory » and is probably going to be the solution. However, I got blocked by an issue: arrays. For uniforms, it’s currently possible to do something like Foo<B, [i32; 134]>, where 134 is actually any length. There is no way to add that to the SomeBackendStuff trait: it would require a feature Rust doesn’t have (that Haskell does), which is called Quantified Constraints. In Rust, it would be similar to writing something like:

trait SomeBackendStuff: FooBackend<u32> + for<const N: usize> FooBackend<[u32; N]> + …

That would honestly be fantastic, but that’s not likely to ever be available and there is not even an RFC about that but an issue open at rust-lang/rfcs#1481.

To solve this problem with current Rust, I tried with a facade trait, like so:

trait StuffFacade {
  type Foo<T> were Self: FooBackend<T>;
  type FooArray<T, const N: usize> where Self: FooBackend<[T; N]>;
}

This works, but is not really elegant, and requires GAT (rust-lang/rust#44265) and currently a nightly compiler.

For this reason, I’m going to let the situation like this until GAT are stabilized and, possibly, where HRTBs work for higher-kinded types (i.e. for<T> …).

hadronized added a commit that referenced this issue Apr 11, 2022
The implementor and the type variable are now swapped. Instead of
implementing it as:

```rust
impl Uniformable<TheBackendType> for [f32; 3] { /* … */ }
```

It is now implemented as:

```rust
impl Uniformable<[f32; 3]> for TheBackendType { /* … */ }
```

This allows for better polymorphic code. Next commit should introduce a
type family to be able to manipulate references correctly, and use the
`luminance::shader::types` types, since the current implementors are
wrong and ambiguous.

Relates to #438.
hadronized added a commit that referenced this issue Apr 11, 2022
This commit provides support for existential in `Uniform<T>`, so that
it’s possible to ask for things like `&'a [T; N]`, where `'a` comes from
call-site.

The current implementation is not perfect as it would be normally
implemented with GATs… but I don’t have GATs.

Relates to #438.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Performance enhancement, documentation, adding tests, etc. hard An hard feature / bug.
Projects
None yet
Development

No branches or pull requests

3 participants