From c6084582479143e67a7e65bda73d5f99476c65c7 Mon Sep 17 00:00:00 2001
From: Corwin <corwin@kuiper.dev>
Date: Wed, 5 Apr 2023 19:29:29 +0100
Subject: [PATCH] start on writing docs. A long way to go...

---
 agb/src/display/mod.rs                        |  1 -
 agb/src/display/object.rs                     | 13 +++-
 agb/src/display/object/affine.rs              |  8 ++
 agb/src/display/object/sprites.rs             |  2 +-
 agb/src/display/object/sprites/sprite.rs      |  1 +
 .../object/sprites/sprite_allocator.rs        | 74 +++++++++++++------
 .../display/object/unmanaged/attributes.rs    |  3 +
 agb/src/display/palette16.rs                  |  1 -
 8 files changed, 77 insertions(+), 26 deletions(-)

diff --git a/agb/src/display/mod.rs b/agb/src/display/mod.rs
index bc6f6d16..0d6dd6d2 100644
--- a/agb/src/display/mod.rs
+++ b/agb/src/display/mod.rs
@@ -16,7 +16,6 @@ pub mod bitmap3;
 pub mod bitmap4;
 /// Test logo of agb.
 pub mod example_logo;
-/// Implements sprites.
 pub mod object;
 /// Palette type.
 pub mod palette16;
diff --git a/agb/src/display/object.rs b/agb/src/display/object.rs
index 281b8253..dfecf718 100644
--- a/agb/src/display/object.rs
+++ b/agb/src/display/object.rs
@@ -1,10 +1,21 @@
+#![warn(missing_docs)]
+//! # Sprites and objects
+//!
+//! There are two implementations of objects depending on how you want to make
+//! your game. There is the *Managed* and *Unmanaged* systems, given by
+//! [OamManaged] and [OamUnmanaged] respectively. The managed Oam is easier to
+//! use and has built in support for setting the `z` coordinate. The unmanaged
+//! Oam is simpler and more efficient with the tradeoff that it is slightly
+//! harder to integrate into your games depending on how they are architectured.
+
 mod affine;
 mod managed;
 mod sprites;
 mod unmanaged;
 
 pub use sprites::{
-    include_aseprite, DynamicSprite, Graphics, Size, Sprite, SpriteLoader, SpriteVram, Tag, TagMap,
+    include_aseprite, DynamicSprite, Graphics, PaletteVram, Size, Sprite, SpriteLoader, SpriteVram,
+    Tag, TagMap,
 };
 
 pub use affine::AffineMatrixInstance;
diff --git a/agb/src/display/object/affine.rs b/agb/src/display/object/affine.rs
index 703ab160..f10b9007 100644
--- a/agb/src/display/object/affine.rs
+++ b/agb/src/display/object/affine.rs
@@ -14,6 +14,11 @@ struct AffineMatrixData {
 #[derive(Debug, Clone)]
 pub(crate) struct AffineMatrixVram(Rc<AffineMatrixData>);
 
+/// An affine matrix that can be used on objects. It is just in time copied to
+/// vram, so you can have as many as you like of these but you can only use up
+/// to 16 in one frame. They are reference counted (Cloning is cheap) and
+/// immutable, if you want to change a matrix you must make a new one and set it
+/// on all your objects.
 #[derive(Debug, Clone)]
 pub struct AffineMatrixInstance {
     location: AffineMatrixVram,
@@ -21,6 +26,9 @@ pub struct AffineMatrixInstance {
 
 impl AffineMatrixInstance {
     #[must_use]
+    /// Creates an instance of an affine matrix from its object form. Check out
+    /// the docs for [AffineMatrix][crate::display::affine::AffineMatrix] to see
+    /// how you can use them to create effects.
     pub fn new(affine_matrix: AffineMatrixObject) -> AffineMatrixInstance {
         AffineMatrixInstance {
             location: AffineMatrixVram(Rc::new(AffineMatrixData {
diff --git a/agb/src/display/object/sprites.rs b/agb/src/display/object/sprites.rs
index c384be6a..d9b3d9cf 100644
--- a/agb/src/display/object/sprites.rs
+++ b/agb/src/display/object/sprites.rs
@@ -4,4 +4,4 @@ mod sprite_allocator;
 const BYTES_PER_TILE_4BPP: usize = 32;
 
 pub use sprite::{include_aseprite, Graphics, Size, Sprite, Tag, TagMap};
-pub use sprite_allocator::{DynamicSprite, SpriteLoader, SpriteVram};
+pub use sprite_allocator::{DynamicSprite, PaletteVram, SpriteLoader, SpriteVram};
diff --git a/agb/src/display/object/sprites/sprite.rs b/agb/src/display/object/sprites/sprite.rs
index 49774897..6751c47d 100644
--- a/agb/src/display/object/sprites/sprite.rs
+++ b/agb/src/display/object/sprites/sprite.rs
@@ -28,6 +28,7 @@ impl Sprite {
     }
 
     #[must_use]
+    /// Gives the size of the sprite
     pub fn size(&self) -> Size {
         self.size
     }
diff --git a/agb/src/display/object/sprites/sprite_allocator.rs b/agb/src/display/object/sprites/sprite_allocator.rs
index 0c8d02a2..ae28f908 100644
--- a/agb/src/display/object/sprites/sprite_allocator.rs
+++ b/agb/src/display/object/sprites/sprite_allocator.rs
@@ -87,14 +87,17 @@ impl Drop for PaletteVramData {
     }
 }
 
-#[derive(Debug)]
+/// A palette in vram, this is reference counted so it is cheap to Clone.
+#[derive(Debug, Clone)]
 pub struct PaletteVram {
     data: Rc<PaletteVramData>,
 }
 
 impl PaletteVram {
-    fn new(palette: &Palette16) -> Option<PaletteVram> {
-        let allocated = unsafe { PALETTE_ALLOCATOR.alloc(Palette16::layout()) }?;
+    /// Attempts to allocate a new palette in sprite vram
+    pub fn new(palette: &Palette16) -> Result<PaletteVram, LoaderError> {
+        let allocated = unsafe { PALETTE_ALLOCATOR.alloc(Palette16::layout()) }
+            .ok_or(LoaderError::PaletteFull)?;
 
         unsafe {
             allocated
@@ -103,7 +106,7 @@ impl PaletteVram {
                 .copy_from_nonoverlapping(palette.colours.as_ptr(), palette.colours.len());
         }
 
-        Some(PaletteVram {
+        Ok(PaletteVram {
             data: Rc::new(PaletteVramData {
                 location: Location::from_palette_ptr(allocated),
             }),
@@ -124,20 +127,37 @@ impl Drop for SpriteVramData {
     }
 }
 
+#[non_exhaustive]
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+pub enum LoaderError {
+    SpriteFull,
+    PaletteFull,
+}
+
+/// A sprite that is currently loaded into vram.
+///
+/// This is referenced counted such that clones of this are cheap and can be
+/// reused between objects. When nothing references the sprite it gets
+/// deallocated from vram.
+///
+/// You can create one of these either via the [DynamicSprite] interface, which
+/// allows you to generate sprites at run time, or via a [SpriteLoader] (or
+/// [OamManaged][super::super::OamManaged]).
 #[derive(Clone, Debug)]
 pub struct SpriteVram {
     data: Rc<SpriteVramData>,
 }
 
 impl SpriteVram {
-    fn new(data: &[u8], size: Size, palette: PaletteVram) -> Option<SpriteVram> {
-        let allocated = unsafe { SPRITE_ALLOCATOR.alloc(size.layout()) }?;
+    fn new(data: &[u8], size: Size, palette: PaletteVram) -> Result<SpriteVram, LoaderError> {
+        let allocated =
+            unsafe { SPRITE_ALLOCATOR.alloc(size.layout()) }.ok_or(LoaderError::SpriteFull)?;
         unsafe {
             allocated
                 .as_ptr()
                 .copy_from_nonoverlapping(data.as_ptr(), data.len());
         }
-        Some(SpriteVram {
+        Ok(SpriteVram {
             data: Rc::new(SpriteVramData {
                 location: Location::from_sprite_ptr(allocated),
                 size,
@@ -163,19 +183,19 @@ impl SpriteLoader {
     fn create_sprite_no_insert(
         palette_map: &mut HashMap<PaletteId, Weak<PaletteVramData>>,
         sprite: &'static Sprite,
-    ) -> Option<(Weak<SpriteVramData>, SpriteVram)> {
+    ) -> Result<(Weak<SpriteVramData>, SpriteVram), LoaderError> {
         let palette = Self::try_get_vram_palette_asoc(palette_map, sprite.palette)?;
 
         let sprite = SpriteVram::new(sprite.data, sprite.size, palette)?;
-        Some((Rc::downgrade(&sprite.data), sprite))
+        Ok((Rc::downgrade(&sprite.data), sprite))
     }
 
     fn try_get_vram_palette_asoc(
         palette_map: &mut HashMap<PaletteId, Weak<PaletteVramData>>,
         palette: &'static Palette16,
-    ) -> Option<PaletteVram> {
+    ) -> Result<PaletteVram, LoaderError> {
         let id = PaletteId::from_static_palette(palette);
-        Some(match palette_map.entry(id) {
+        Ok(match palette_map.entry(id) {
             crate::hash_map::Entry::Occupied(mut entry) => match entry.get().upgrade() {
                 Some(data) => PaletteVram { data },
                 None => {
@@ -192,12 +212,16 @@ impl SpriteLoader {
         })
     }
 
-    pub fn try_get_vram_sprite(&mut self, sprite: &'static Sprite) -> Option<SpriteVram> {
+    /// Attempts to get a sprite
+    pub fn try_get_vram_sprite(
+        &mut self,
+        sprite: &'static Sprite,
+    ) -> Result<SpriteVram, LoaderError> {
         // check if we already have the sprite in vram
 
         let id = SpriteId::from_static_sprite(sprite);
 
-        Some(match self.static_sprite_map.entry(id) {
+        Ok(match self.static_sprite_map.entry(id) {
             crate::hash_map::Entry::Occupied(mut entry) => match entry.get().upgrade() {
                 Some(data) => SpriteVram { data },
                 None => {
@@ -216,18 +240,24 @@ impl SpriteLoader {
         })
     }
 
-    pub fn try_get_vram_palette(&mut self, palette: &'static Palette16) -> Option<PaletteVram> {
+    /// Attempts to allocate a static palette
+    pub fn try_get_vram_palette(
+        &mut self,
+        palette: &'static Palette16,
+    ) -> Result<PaletteVram, LoaderError> {
         Self::try_get_vram_palette_asoc(&mut self.static_palette_map, palette)
     }
 
+    /// Allocates a sprite to vram, panics if it cannot fit.
     pub fn get_vram_sprite(&mut self, sprite: &'static Sprite) -> SpriteVram {
         self.try_get_vram_sprite(sprite)
-            .expect("no free sprite slots")
+            .expect("cannot create sprite")
     }
 
+    /// Allocates a palette to vram, panics if it cannot fit.
     pub fn get_vram_palette(&mut self, palette: &'static Palette16) -> PaletteVram {
         self.try_get_vram_palette(palette)
-            .expect("no free palette slots")
+            .expect("cannot create sprite")
     }
 
     pub(crate) fn new() -> Self {
@@ -237,6 +267,9 @@ impl SpriteLoader {
         }
     }
 
+    /// Remove internal references to sprites that no longer exist in vram. If
+    /// you neglect calling this, memory will leak over time in relation to the
+    /// total number of different sprites used. It will not leak vram.
     pub fn garbage_collect(&mut self) {
         self.static_sprite_map
             .retain(|_, v| Weak::strong_count(v) != 0);
@@ -277,18 +310,15 @@ impl DynamicSprite<'_> {
         DynamicSprite { data, size }
     }
 
-    #[must_use]
     /// Tries to copy the sprite to vram to be used to set object sprites.
-    /// Returns None if there is no room in sprite vram.
-    pub fn try_vram(&self, palette: PaletteVram) -> Option<SpriteVram> {
+    pub fn try_vram(&self, palette: PaletteVram) -> Result<SpriteVram, LoaderError> {
         SpriteVram::new(self.data, self.size, palette)
     }
 
     #[must_use]
     /// Tries to copy the sprite to vram to be used to set object sprites.
-    /// Panics if there is no room in sprite vram.
+    /// Panics if it cannot be allocated.
     pub fn to_vram(&self, palette: PaletteVram) -> SpriteVram {
-        self.try_vram(palette)
-            .expect("No slot for sprite available")
+        self.try_vram(palette).expect("cannot create sprite")
     }
 }
diff --git a/agb/src/display/object/unmanaged/attributes.rs b/agb/src/display/object/unmanaged/attributes.rs
index 7223d6ae..2ae52059 100644
--- a/agb/src/display/object/unmanaged/attributes.rs
+++ b/agb/src/display/object/unmanaged/attributes.rs
@@ -26,8 +26,11 @@ impl Default for Attributes {
 }
 
 #[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)]
+/// The affine mode
 pub enum AffineMode {
+    /// Normal affine, this is where the area of the affine is equal to the sprite size
     Affine = 1,
+    /// Double affine, this is where the area of the affine is double that of the sprite
     AffineDouble = 3,
 }
 
diff --git a/agb/src/display/palette16.rs b/agb/src/display/palette16.rs
index 449eb31c..7296b5bd 100644
--- a/agb/src/display/palette16.rs
+++ b/agb/src/display/palette16.rs
@@ -14,7 +14,6 @@ impl Palette16 {
 
     // Clippy bug: claims that index is only used in recursion. I can't reproduce in
     // other examples, even just copy pasting this struct and impl into a blank project :/
-    #[allow(clippy::only_used_in_recursion)]
     pub fn update_colour(&mut self, index: usize, colour: u16) {
         self.colours[index] = colour;
     }