Updates

[dakom]

No description provided.

[Copilot]

Pull request overview

This PR improves renderer robustness by making several dynamic buffer writes fallible (instead of silently ignoring/panicking), adding explicit overflow checks for glTF geometry slicing, and expanding development docs for PNG-based skybox workflows.

Changes:

• Make DynamicStorageBuffer::update return a Result and plumb capacity/overflow errors up through meshes/skins/morphs/instances.
• Add checked byte-size helpers for mesh buffer vertex info and use them in glTF mesh population to avoid usize overflow.
• Update development documentation with PNG skybox generation and KTX packaging steps.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
docs/DEVELOPMENT.md	Adds PNG skybox and KTX2 packaging instructions.
crates/renderer/src/meshes/skins.rs	Propagates storage buffer update failures via AwsmSkinError.
crates/renderer/src/meshes/morphs.rs	Propagates storage buffer update failures via AwsmMeshError.
crates/renderer/src/meshes/error.rs	Adds AwsmMeshError::BufferCapacityOverflow.
crates/renderer/src/meshes/buffer_info.rs	Adds checked size helpers and documents overflow panics.
crates/renderer/src/meshes.rs	Propagates storage buffer update failures and makes instancing insert fallible.
crates/renderer/src/materials.rs	Logs material buffer update failures instead of ignoring them.
crates/renderer/src/instances.rs	Makes transform insertion fallible and adds instance overflow error variant.
crates/renderer/src/gltf/populate/mesh.rs	Adds checked arithmetic for geometry byte slicing and new error variant.
crates/renderer/src/gltf/populate/extensions/instancing.rs	Maps instancing buffer overflow into glTF extension error.
crates/renderer/src/gltf/error.rs	Adds GeometryDataSizeOverflow error.
crates/renderer/src/buffer/dynamic_uniform.rs	Adds clear() resetting to initial capacity.
crates/renderer/src/buffer/dynamic_storage.rs	Makes updates fallible, adds capacity ceiling checks, and adds a new overflow-related test.

Comments suppressed due to low confidence (3)

crates/renderer/src/buffer/dynamic_storage.rs:114

• DynamicStorageBuffer::update changed its return type from usize to Result<usize, DynamicStorageBufferError> and the method/type is pub via crate::buffer::dynamic_storage. If this crate is published/consumed externally, this is a breaking API change. Consider providing a compatibility wrapper (e.g., update_unchecked that panics like before) or ensuring the versioning/changelog reflects the break.

    /// Updates or inserts data for the given key.
    ///
    /// If the key exists and the new data fits in the existing allocation,
    /// it reuses the same memory. Otherwise, it reallocates.
    ///
    /// Returns the byte offset of the data in the buffer, or an error if the
    /// buffer cannot grow large enough to accommodate the data.
    pub fn update(&mut self, key: K, bytes: &[u8]) -> Result<usize, DynamicStorageBufferError> {
        // remove & reinsert if new size doesn’t fit existing block
        if let Some((off, old_size)) = self.slot_indices.get(key).copied() {
            if bytes.len() <= old_size {
                self.raw_data[off..off + bytes.len()].copy_from_slice(bytes);

crates/renderer/src/buffer/dynamic_storage.rs:128

• DynamicStorageBuffer::update removes the existing allocation before attempting the fallible re-insert. If insert fails (e.g., capacity ceiling), the key is left removed and the previous data is lost. Consider making this operation atomic on failure (e.g., attempt grow/allocation first, or reinsert/restore the old allocation if insertion fails).

                // clear unused tail
                if bytes.len() < old_size {
                    self.raw_data[off + bytes.len()..off + old_size].fill(ZERO);
                }
                self.mark_dirty_range(off, old_size);
                return Ok(off);
            }
            self.remove(key);
        }
        self.insert(key, bytes)
    }

    /// Updates existing data using a callback, without reallocation.
    ///

crates/renderer/src/buffer/dynamic_storage.rs:1420

• test_capacity_overflow_returns_error doesn’t actually exercise the overflow/error path: it updates with 100 bytes (which succeeds) and then formats an error enum variant. This can pass even if update never returns Err for real overflow cases. Consider either removing the test, or restructuring to test a reachable failure mode (e.g., unit-test the size/rounding logic directly, or expose a helper for computing the required allocation size and test that it returns CapacityOverflow on overflow).

        // We can't actually allocate that much, but we can test the checked arithmetic
        // by verifying the error type exists and small allocations still work
        let result = buffer.update(key1, &[0u8; 100]);
        assert!(result.is_ok());

        // Verify the error type is correct
        let err = DynamicStorageBufferError::CapacityOverflow;
        assert!(format!("{err}").contains("capacity overflow"));
    }
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The section header “Skybox (with mips?)” is ambiguous and reads like a placeholder. Consider renaming to something definitive (e.g., “Skybox (with mipmaps)”) and/or briefly stating when to prefer the mipmapped EXR path vs the PNG path.

[Copilot]

transform_insert mutates cpu_transforms before performing the now-fallible buffer update. If the buffer update fails, the method returns Err but leaves cpu_transforms (and potentially other maps) partially updated. Consider updating the GPU buffer first (or rolling back the CPU-side inserts on error) so the operation is all-or-nothing.

[Copilot]

Materials::update now logs storage-buffer update failures but still leaves the in-memory material mutated. On Err, the GPU buffer contents remain stale and subsequent rendering may use outdated uniform data for that key. Consider making this operation fallible (return Result), or rolling back the material mutation / marking the material as invalid when the buffer update fails.

[Copilot]

transform_insert changed from returning () to Result<()> and the method is pub on a pub type. If Instances is part of the crate’s public API, this is a breaking change for downstream users. Consider either bumping the crate version appropriately, or adding a new fallible method (e.g., try_transform_insert) while keeping the old signature for compatibility.

[Copilot]

Skins::insert inserts into skeleton_transforms before performing the now-fallible buffer updates. If either buffer update fails, the function returns Err but leaves a partially-initialized skin key in skeleton_transforms (and possibly sets_len / inverse_bind_matrices). Consider reordering so fallible allocations happen first, or add rollback/cleanup on error to avoid leaking inconsistent state.

[Copilot]

insert_raw inserts the morph Info into self.infos before performing the now-fallible storage buffer updates. If weights.update or values.update fails, the method returns Err but the infos entry remains, leaving an orphaned key. Consider performing the fallible buffer updates first (using a temporary key) or removing the infos entry on error.

[Copilot]

insert_resource inserts a MeshResource into self.resources before doing several now-fallible buffer updates. If any .update(...) returns Err, the function exits early and leaves a partially initialized resource entry (offsets remain defaults) and potentially some buffers updated. Consider allocating/updating buffers first and only inserting the resource record after success, or explicitly cleaning up/removing the resource on error.

[Copilot]

The newly added PNG instructions use inconsistent capitalization/grammar (e.g., “or, if simple png”). Consider rephrasing to something like “Or, for a simple PNG:” and capitalizing “PNG” for readability/consistency with the rest of the doc.

Suggested change

	or, if simple png
	Or, for a simple PNG: