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

[Feature] Indexed drawing #42

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

0x00002a
Copy link
Contributor

@0x00002a 0x00002a commented Feb 1, 2024

This adds a wrapper for C3D_DrawElements which is index based drawing. It is marked unsafe for reasons which are explained in the doc comment and #41. I've also added an example to both test it works and demonstrate its usage

@Meziu
Copy link
Member

Meziu commented Feb 1, 2024

Quick drop-in opinion: I don’t think it makes sense to have unsafe wrappers to what could definitely be safe functions. I would much rather these functions to be defined using the already existing wrapping functionality (like LinearAllocator requirements rather than generic “pointer is linear” functions). Not saying these tools aren’t useful or good, but the objective of this crate is to provide a safe overlay, above all things.

@0x00002a
Copy link
Contributor Author

0x00002a commented Feb 1, 2024

Quick drop-in opinion: I don’t think it makes sense to have unsafe wrappers to what could definitely be safe functions. I would much rather these functions to be defined using the already existing wrapping functionality (like LinearAllocator requirements rather than generic “pointer is linear” functions). Not saying these tools aren’t useful or good, but the objective of this crate is to provide a safe overlay, above all things.

Yeah so I agree on both points. The first one see #41 for the issues there, for the second one I would like to statically ensure that its in linear memory but I don't think we can do that without requiring some kind of wrapper like LinearVec or something (which is actually what I do in my downstream code, but thats not a library so it can be more opinionated about data storage)

citro3d/src/lib.rs Show resolved Hide resolved
);
}

//instance.draw_arrays(buffer::Primitive::Triangles, vbo_data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just leftover from copy-pasting the existing triangle example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good catch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might call this cube-indexed.rs or something just to make it clear that it uses an index buffer as compared to the existing triangle xample

Comment on lines +145 to +146
let mut indecies = Vec::with_capacity_in(indecies_a.len(), ctru::linear::LinearAllocator);
indecies.extend_from_slice(&indecies_a);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe to enforce lifetimes for index buffers we could have something like buffer::Info::index_buffer(&[usize]). I think if we had that it could resolve at least partially some of the lifetime issues, since Slice already captures the lifetime of the vertex buffer. Then I think the code would look something like this?

let (attr_info, _vbo_data) = prepare_vbos(&mut buf_info, &vbo_data);

let mut index_buffer = buf_info.index_buffer();
// This possibly also checks that all  the new indices are valid for the vertex data:
index_buffer.extend_from_slice(&indices_a);

Edit: actually, I guess we'd also need to make sure the vertex buffer was added at some point, so maybe this would be a method on buffer::Slice instead, or a new buffer::Info::add_indexed(vbo_data, attrib_info, indices) or something. I'm not sure what the best API would be but maybe something along those lines

@@ -0,0 +1,6 @@
/// Check if pointer is in linear memory
pub fn is_linear_ptr<P>(p: *const P) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does seem useful to have — I've also thought about having some kind of type safety around this, e.g.

trait Linear {}

impl<T> Linear for Vec<T, LinearAllocator> {}

The problem is that other APIs would end up with somewhat inconvenient definitions, e.g.

    pub fn add<'this, 'a, T, V>(
        &'this mut self,
        vbo_data: &T,
        attrib_info: &attrib::Info,
    ) -> crate::Result<Slice<'a>>
    where
        'this: 'a,
        T: Deref<Target = [V]> + Linear,
    {

But maybe the type safety would be worth it and then we wouldn't need panics or Err types for all these things that require linear allocation.

Comment on lines +169 to +171
/// # Safety
/// If `indices` goes out of scope before the current frame ends it will cause a use-after-free (possibly by the GPU)
/// If `buf` does not contain all the vertices references by `indices` it will cause an invalid access by the GPU (this crashes citra)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm fine with having a new unsafe API for now, but definitely would like to see if we can solve this with the type system somehow. I mentioned a possible option for the second issue in my other comment and I think the first issue could be solved with other stuff for #41 so I think we at least have a path forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants