API of Instance is fundamentally unsound

[0x00002a]

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:

• Dropping Instance in the wrong order causes a crash on exit #35
• C3D_BindProgram stores a pointer to a Program (use-after-free in bind_program) #37
• Texenvs need to stay alive until they're drawn #36
• draw_arrays doesn't require the underlying 'vbo to outlive the frame end (which will cause use-after-free)

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]

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]

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

[Bash-09]

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:

• Configuring the entire GPU environment ensures that no previous state (that may have been dropped) can be accidentally used later
  • Once a frame is over, you are free to dispose of previously used data, as future render calls will not continue to use it
• All the state required for a draw call is guaranteed to be set, so it is more difficult to forget to set something and then have a draw call fail or not do anything
• Makes it easier to tell what state the GPU is in without having to trace all your calls to bind and set things
• All data used in the render call is borrowed by the renderpass so it must be alive until the end of the frame
  • Resolves lifetime issues!!

Some drawbacks:

• Each draw call is doing more setup than necessary
  • Many draw calls will probably continue to use almost the same state, so it's not always necessary to reconfigure everything
    • At least I believe a lot of those calls into C are quite small, just setting some pointers or flags, so I don't think it would have a significant performance impact. Perhaps there could be optimisations in the future to avoid updating more than necessary, but at this point I think that would be a bit premature
• Not as low-level control over the raw API
• As more features are added, the RenderPass struct could potentially get quite large

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.