From 8f9d70a9e8f9b317109483d0de2f152abcd603c1 Mon Sep 17 00:00:00 2001 From: Ian Chamberlain Date: Wed, 19 Jun 2024 14:45:30 -0400 Subject: [PATCH] Use a checked index API and do a bit of cleanup --- citro3d/examples/cube.rs | 40 ++++++++++------------ citro3d/src/buffer.rs | 59 ++++++++++++++++++++++++++++++++ citro3d/src/error.rs | 8 +++++ citro3d/src/lib.rs | 72 ++++++++++++++-------------------------- citro3d/src/util.rs | 6 ---- 5 files changed, 110 insertions(+), 75 deletions(-) delete mode 100644 citro3d/src/util.rs diff --git a/citro3d/examples/cube.rs b/citro3d/examples/cube.rs index 69d9f80..980f8da 100644 --- a/citro3d/examples/cube.rs +++ b/citro3d/examples/cube.rs @@ -8,8 +8,7 @@ use citro3d::math::{ AspectRatio, ClipPlanes, CoordinateOrientation, FVec3, Matrix4, Projection, StereoDisplacement, }; use citro3d::render::ClearFlags; -use citro3d::{attrib, buffer, render, shader}; -use citro3d::{texenv, IndexType}; +use citro3d::{attrib, buffer, render, shader, texenv}; use ctru::prelude::*; use ctru::services::gfx::{RawFrameBuffer, Screen, TopScreen3D}; @@ -86,17 +85,20 @@ fn main() { let (mut top_left, mut top_right) = top_screen.split_mut(); let RawFrameBuffer { width, height, .. } = top_left.raw_framebuffer(); - let mut top_left_target = - render::Target::new(width, height, top_left, None).expect("failed to create render target"); + let mut top_left_target = instance + .render_target(width, height, top_left, None) + .expect("failed to create render target"); let RawFrameBuffer { width, height, .. } = top_right.raw_framebuffer(); - let mut top_right_target = render::Target::new(width, height, top_right, None) + let mut top_right_target = instance + .render_target(width, height, top_right, None) .expect("failed to create render target"); let mut bottom_screen = gfx.bottom_screen.borrow_mut(); let RawFrameBuffer { width, height, .. } = bottom_screen.raw_framebuffer(); - let mut bottom_target = render::Target::new(width, height, bottom_screen, None) + let mut bottom_target = instance + .render_target(width, height, bottom_screen, None) .expect("failed to create bottom screen render target"); let shader = shader::Library::from_bytes(SHADER_BYTES).unwrap(); @@ -111,13 +113,17 @@ fn main() { y: v[1], z: v[2], }, - color: Vec3::new(1.0, 0.7, 0.5), + color: { + // Give each vertex a slightly different color just to highlight edges/corners + let value = i as f32 / VERTS.len() as f32; + Vec3::new(1.0, 0.7 * value, 0.5) + }, }) { vbo_data.push(vert); } let mut buf_info = buffer::Info::new(); - let (attr_info, vbo_data) = prepare_vbos(&mut buf_info, &vbo_data); + let (attr_info, vbo_slice) = prepare_vbos(&mut buf_info, &vbo_data); // 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 @@ -134,7 +140,7 @@ fn main() { FVec3::new(0.0, 1.0, 0.0), CoordinateOrientation::RightHanded, ); - let indecies_a = [ + let indices: &[u8] = &[ 0, 3, 1, 1, 3, 2, // triangles making up the top (+y) facing side. 4, 5, 7, 5, 6, 7, // bottom (-y) 8, 11, 9, 9, 11, 10, // right (+x) @@ -142,8 +148,7 @@ fn main() { 16, 19, 17, 17, 19, 18, // back (+z) 20, 21, 23, 21, 22, 23, // forward (-z) ]; - let mut indecies = Vec::with_capacity_in(indecies_a.len(), ctru::linear::LinearAllocator); - indecies.extend_from_slice(&indecies_a); + let indices = vbo_slice.index_buffer(indices).unwrap(); while apt.main_loop() { hid.scan_input(); @@ -160,21 +165,12 @@ fn main() { .select_render_target(target) .expect("failed to set render target"); - instance.bind_vertex_uniform( - projection_uniform_idx, - &(projection * camera_transform.clone()), - ); + instance.bind_vertex_uniform(projection_uniform_idx, projection * camera_transform); instance.set_attr_info(&attr_info); unsafe { - instance.draw_elements( - buffer::Primitive::Triangles, - &buf_info, - IndexType::U16(&indecies), - ); + instance.draw_elements(buffer::Primitive::Triangles, &buf_info, &indices); } - - //instance.draw_arrays(buffer::Primitive::Triangles, vbo_data); }; let Projections { diff --git a/citro3d/src/buffer.rs b/citro3d/src/buffer.rs index 0a19ff3..0785237 100644 --- a/citro3d/src/buffer.rs +++ b/citro3d/src/buffer.rs @@ -5,7 +5,10 @@ use std::mem::MaybeUninit; +use ctru::linear::LinearAllocator; + use crate::attrib; +use crate::Error; /// Vertex buffer info. This struct is used to describe the shape of the buffer /// data to be sent to the GPU for rendering. @@ -46,6 +49,62 @@ impl Slice<'_> { pub fn info(&self) -> &Info { self.buf_info } + + /// Get an index buffer for this slice using the given indices. + /// + /// # Errors + /// + /// Returns an error if: + /// - any of the given indices are out of bounds. + /// - the given slice is too long for its length to fit in a `libc::c_int`. + pub fn index_buffer(&self, indices: &[I]) -> Result, Error> + where + I: Index + Copy + Into, + { + if libc::c_int::try_from(indices.len()).is_err() { + return Err(Error::InvalidSize); + } + + for &idx in indices { + let idx = idx.into(); + let len = self.len(); + if idx >= len { + return Err(Error::IndexOutOfBounds { idx, len }); + } + } + + Ok(unsafe { self.index_buffer_unchecked(indices) }) + } + + /// Get an index buffer for this slice using the given indices without + /// bounds checking. + /// + /// # Safety + /// + /// If any indices are outside this buffer it can cause an invalid access by the GPU + /// (this crashes citra). + pub unsafe fn index_buffer_unchecked( + &self, + indices: &[I], + ) -> Vec { + let mut buf = Vec::with_capacity_in(indices.len(), LinearAllocator); + buf.extend_from_slice(indices); + buf + } +} + +/// A type that can be used as an index for indexed drawing. +pub trait Index: crate::private::Sealed { + /// The data type of the index, as used by [`citro3d_sys::C3D_DrawElements`]'s `type_` parameter. + const TYPE: libc::c_int; +} + +impl Index for u8 { + const TYPE: libc::c_int = citro3d_sys::C3D_UNSIGNED_BYTE as _; +} + +impl Index for u16 { + const TYPE: libc::c_int = citro3d_sys::C3D_UNSIGNED_SHORT as _; } /// The geometric primitive to draw (i.e. what shapes the buffer data describes). diff --git a/citro3d/src/error.rs b/citro3d/src/error.rs index 9a99089..bcb69bb 100644 --- a/citro3d/src/error.rs +++ b/citro3d/src/error.rs @@ -1,5 +1,6 @@ //! General-purpose error and result types returned by public APIs of this crate. +use core::fmt; use std::ffi::NulError; use std::num::TryFromIntError; use std::sync::TryLockError; @@ -36,6 +37,13 @@ pub enum Error { InvalidName, /// The requested resource could not be found. NotFound, + /// Attempted to use an index that was out of bounds. + IndexOutOfBounds { + /// The index used. + idx: libc::c_int, + /// The length of the collection. + len: libc::c_int, + }, } impl From for Error { diff --git a/citro3d/src/lib.rs b/citro3d/src/lib.rs index b17a73c..c20155e 100644 --- a/citro3d/src/lib.rs +++ b/citro3d/src/lib.rs @@ -1,5 +1,6 @@ #![feature(custom_test_frameworks)] #![test_runner(test_runner::run_gdb)] +#![feature(allocator_api)] #![feature(doc_cfg)] #![feature(doc_auto_cfg)] #![doc(html_root_url = "https://rust3ds.github.io/citro3d-rs/crates")] @@ -24,15 +25,14 @@ pub mod render; pub mod shader; pub mod texenv; pub mod uniform; -mod util; use std::cell::{OnceCell, RefMut}; use std::fmt; use std::rc::Rc; +use ctru::linear::LinearAllocation; use ctru::services::gfx::Screen; pub use error::{Error, Result}; -use util::is_linear_ptr; use self::texenv::TexEnv; use self::uniform::Uniform; @@ -42,6 +42,12 @@ pub mod macros { pub use citro3d_macros::*; } +mod private { + pub trait Sealed {} + impl Sealed for u8 {} + impl Sealed for u16 {} +} + /// The single instance for using `citro3d`. This is the base type that an application /// should instantiate to use this library. #[non_exhaustive] @@ -205,36 +211,33 @@ impl Instance { /// Draws the vertices in `buf` indexed by `indices`. `indices` must be linearly allocated /// /// # 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) + // TODO: #41 might be able to solve this: + /// If `indices` goes out of scope before the current frame ends it will cause a + /// use-after-free (possibly by the GPU). /// /// # Panics - /// If `indices` is not allocated in linear memory + /// + /// If the given index buffer is too long to have its length converted to `i32`. #[doc(alias = "C3D_DrawElements")] - pub unsafe fn draw_elements<'a>( + pub unsafe fn draw_elements( &mut self, primitive: buffer::Primitive, buf: &buffer::Info, - indices: impl Into>, - ) { + indices: &Indices, + ) where + I: buffer::Index, + Indices: AsRef<[I]> + LinearAllocation, + { self.set_buffer_info(buf); - let indices: IndexType<'a> = indices.into(); - let elements = match indices { - IndexType::U16(v) => v.as_ptr() as *const _, - IndexType::U8(v) => v.as_ptr() as *const _, - }; - assert!( - is_linear_ptr(elements), - "draw_elements requires linear allocated indices buffer" - ); + + let indices = indices.as_ref(); + let elements = indices.as_ptr().cast(); + citro3d_sys::C3D_DrawElements( primitive as ctru_sys::GPU_Primitive_t, - indices.len() as i32, + indices.len().try_into().unwrap(), // flag bit for short or byte - match indices { - IndexType::U16(_) => citro3d_sys::C3D_UNSIGNED_SHORT, - IndexType::U8(_) => citro3d_sys::C3D_UNSIGNED_BYTE, - } as i32, + I::TYPE, elements, ); } @@ -321,31 +324,6 @@ impl Drop for RenderQueue { } } -pub enum IndexType<'a> { - U16(&'a [u16]), - U8(&'a [u8]), -} -impl IndexType<'_> { - fn len(&self) -> usize { - match self { - IndexType::U16(a) => a.len(), - IndexType::U8(a) => a.len(), - } - } -} - -impl<'a> From<&'a [u8]> for IndexType<'a> { - fn from(v: &'a [u8]) -> Self { - Self::U8(v) - } -} - -impl<'a> From<&'a [u16]> for IndexType<'a> { - fn from(v: &'a [u16]) -> Self { - Self::U16(v) - } -} - #[cfg(test)] mod tests { use ctru::services::gfx::Gfx; diff --git a/citro3d/src/util.rs b/citro3d/src/util.rs deleted file mode 100644 index 727e8a2..0000000 --- a/citro3d/src/util.rs +++ /dev/null @@ -1,6 +0,0 @@ -/// Check if pointer is in linear memory -pub fn is_linear_ptr

(p: *const P) -> bool { - let addr = p as usize; - addr >= ctru_sys::OS_FCRAM_VADDR as usize - && addr < (ctru_sys::OS_FCRAM_VADDR as usize + ctru_sys::OS_FCRAM_SIZE as usize) -}