rust3ds / citro3d-rs

Rust bindings and safe wrappers for citro3d
https://rust3ds.github.io/citro3d-rs
14 stars 11 forks source link

API of Instance is fundamentally unsound #41

Open 0x00002a opened 10 months ago

0x00002a commented 10 months ago

Hi. I'm @Jhynjhiruu's friend. Just wanna preface this by saying this library has been incredibly useful as a reference when working out citro3d, I really appreciate the effort that's gone into this. Now onto the problems

The interface exposed by Instance is fundamentally unsound, what do I mean by this? Well it makes guarantees to/as safe code that it doesn't uphold. Some examples:

The main cause of use-after-free's is the lack of lifetimes for things which need to stick around until the frame ends (when they will be read by the GPU). In its current form at the very least draw_arrays and bind_program need to be marked unsafe (bind_program especially is really nasty because of its pinning requirements).

Alternatively, the lifetimes can be made to work. It just requires moving the frame stuff to a RAII wrapper rather than exclusively render_frame_with. I've created a prototype/proof of concept here I'm not married to it but it fixes everything but #35 which I left alone because of #38. It also lays the foundation for implementing stuff like textures in a safe way as well (as the lifetimes are now there to enforce it).

ian-h-chamberlain commented 10 months ago

Thanks for pointing out some of the issues! It's definitely a goal of this crate to provide safe and sound APIs for everything (after all, you can just use citro3d-sys if you only want unsafe APIs), but you're right that there is some unsoundness exposed by the safe API surface in its current state.

I've gotten a little busier recently and haven't had as much time to look into some of the other issues, but it's awesome to hear that you've tried to take an approach to solving some of them! Having an RAII Frame is definitely an interesting idea and not something that occurred to me before, but I think it does make sense since it allows you to encapsulate the frame begin and end.

If you'd like to continue that work, feel free to open a draft PR and we can discuss the design in more detail there, or if not maybe I'll get a chance to play around with similar concepts and make some other changes. If I do, I'll CC you both on any changes I make in that area just to make sure it seems like a reasonable design there.

0x00002a commented 10 months ago

Cool, I'll see about opening that PR at some point in the next few days (also very busy)

Bash-09 commented 1 month ago

Hihi. I've been working on different sort of API that aims to resolve these issues that takes quite a different approach to managing the rendering pipeline, so I wanted to share in case it could be useful for this crate at all (this also makes use of the Frame abstraction mentioned earlier).

Most of the creation of the structs needed for rendering remains the same, however setting up the texenv and making a draw call now looks something like this:

    // Configure the first fragment shading substage to just pass through the vertex color
    // See https://www.opengl.org/sdk/docs/man2/xhtml/glTexEnv.xml for more insight
    let stage0 = texenv::TexEnv::new()
        .sources(texenv::Mode::BOTH, texenv::Source::PrimaryColor, None, None);

// ...

        instance.render_frame_with(|frame| {
            for (target, proj) in targets {
                target.clear(ClearFlags::ALL, CLEAR_COLOR, 0);

                let pass = RenderPass::new(&program, target, vbo_data, &attr_info)
                    .with_vertex_uniforms([(projection_uniform_idx, proj.into())])
                    .with_texenv_stages([&stage0]);

                frame.draw(&pass).unwrap();
            }
        });

I've implemented a proof-of-concept at https://github.com/Bash-09/citro3d-rs with an updated example showing this API.

Inspired a little by some parts of the wgpu API, a RenderPass is more of a declerative way to make a draw call, where everything needed for rendering is described in a struct at the time of the draw call, so that all of the calls to e.g. bind the program, set the target, setup the texenv and bind any textures, etc, are done at once. Then after that frame is complete, none of those structs need to hang around any longer as the next draw call will either borrow them again or restructure the GPU state so that they are not used (i.e. no future draw call depends on prior state).

Some benefits of this API structure:

Some drawbacks:

So far I think it is quite ergonomic and intuitive to use, however I'm not entirely sure if it aligns with the design goals of this library. So far the library appears to be a reasonably thin wrapper around the C library, while this would probably be removing it somewhat from that (ofc for a maximally thin wrapper though there are always the raw bindings, so perhaps it is okay to deviate from that slightly?). If anybody wants to have a look, share some thoughts on if this seem reasonable/appropriate for this library, that would be pretty cool.