Skip to content

Fixes.md

Latest commit

 

History

History
153 lines (118 loc) · 5.63 KB

Fixes.md

File metadata and controls

153 lines (118 loc) · 5.63 KB

Good, I've confirmed the key findings. Here's the review, organized by priority:

Code Review — chapter_16 (final implementation)

Bugs / Correctness

1. matrix.rs:216 — Exact float comparison in is_invertible()

// Current — wrong, exact float comparison almost never works
pub fn is_invertible(&self) -> bool {
    !(Matrix::determinant(self.data, 4) == 0.0)
}

// Should use your own float_eq:
pub fn is_invertible(&self) -> bool {
    !float_eq(Matrix::determinant(self.data, 4), 0.0)
}

This is particularly ironic since the whole codebase uses float_eq everywhere else.

2. transformation.rs:216 — rotate_z takes &self, all others take self

// rotate_x, rotate_y, translate, scale, shear all take `self`:
pub fn rotate_x(self, r: f64) -> Transformation { ... }

// rotate_z takes &self — inconsistent, means it can't chain after a consuming method:
pub fn rotate_z(&self, r: f64) -> Transformation { ... }

It compiles because data is Copy, but it breaks the builder idiom — you can't write .rotate_y(...).rotate_z(...) if the former consumed self because the result would be a temporary. Change to self.

3. camera.rs:52 — ray_for_pixel takes &mut self but never mutates

// Current — forces callers to have `mut` unnecessarily
pub fn ray_for_pixel(&mut self, px: f64, py: f64) -> Ray {

// Should be:
pub fn ray_for_pixel(&self, px: f64, py: f64) -> Ray {

This also means render(&mut self) is forced mut for the wrong reason.

Idiomatic Rust

4. world.rs — Several match blocks that should be .map()

// Current (world.rs ~183):
match self.objects.get(index) {
    Some(o) => Some(o.as_ref()),
    None => None,
}

// Idiomatic:
self.objects.get(index).map(|o| o.as_ref())

Same pattern appears in world.rs get_object_by_id() and material.rs pattern lookup.

5. world.rs / group.rs — index loops that should be find_map

// Current — loop with early returns:
for s in &self.objects {
    if s.id() == id { return Some(s.as_ref()); }
    if let Some(c) = s.get_object_by_id(id) { return Some(c); }
}

// Idiomatic:
self.objects.iter().find_map(|s| {
    if s.id() == id { Some(s.as_ref()) }
    else { s.get_object_by_id(id) }
})

6. intersection.rs — Eq + Ord contract violation

PartialEq uses float_eq (approximate), but Eq is implemented, which requires the relation to be exact and transitive. Two values that compare equal via float_eq may not sort consistently. This is technically unsound — if you ever sort intersections and rely on stability, edge cases could bite you. The pragmatic fix is to remove the Eq derive and only use PartialEq/PartialOrd, or switch to bit-for-bit f64 comparison for ordering.

7. canvas.rs — Pre-allocate the PPM string

The PPM buffer starts with no capacity and grows via +=. Since you know the dimensions, pre-allocate:

// Each pixel is 3 values of up to 3 digits + spaces/newlines ≈ 12 chars
let mut buffer = String::with_capacity(self.width * self.height * 12);

Design / Architecture

8. Shape boilerplate — repeated across every shape

Every shape (sphere.rs, plane.rs, cube.rs, etc.) has identical fields and identical trait method bodies:

fn id(&self) -> Uuid { self.id }
fn parent_id(&self) -> Option<Uuid> { self.parent_id }
fn set_parent_id(&mut self, id: Uuid) { self.parent_id = Some(id); }
fn transform(&self) -> Matrix { self.transform }
// ... 5 more identical methods

The idiomatic Rust approach is a helper struct:

pub struct ShapeBase {
    pub id: Uuid,
    pub parent_id: Option<Uuid>,
    pub transform: Matrix,
    pub material: Material,
}

Then each shape holds a base: ShapeBase and the trait impls delegate to it. Or a macro_rules! macro. Either eliminates ~50 lines of copy-paste per shape.

9. obj_file.rs — panics on bad input instead of returning Result

// Current — panics on malformed OBJ files:
let x: f64 = line_iter.next().unwrap().parse().unwrap();

ObjFile::parse_obj_file is a public API reading external files. It should return Result<Group, Box<dyn std::error::Error>> or a custom error type. A bad OBJ file currently crashes the whole program.

10. lib.rs:50 — multiple_array should be named multiply_array

"multiple" is a noun; "multiply" is the verb you want. The function is private so it's low-stakes, but it's also used as an important internal concept throughout transformation.rs.

Minor / Documentation

  • camera.rs:62 — "teh canvas" typo
  • computations.rs:14,17 — "teh surface" twice
  • material.rs:118,131 — "teh the angle", "Add teh"
  • vector.rs:21 — "teh vector"
  • transformation.rs:216 — missing closing /// line on the doc comment for rotate_z (the /// block ends without a closing backtick before the pub fn)

Summary by Priority

# Location Issue Severity
1 matrix.rs:216 == 0.0 float comparison Bug
2 transformation.rs:216 rotate_z takes &self not self Bug
3 camera.rs:52 &mut self should be &self Wrong API
4 intersection.rs Eq + float comparison unsound Correctness
5 obj_file.rs Panics on bad input Robustness
6 world.rs, group.rs match/loop → .map()/find_map() Idiomatic
7 All shapes Boilerplate duplication Maintainability
8 canvas.rs No pre-allocated string capacity Performance
9 lib.rs multiple_array typo in name Minor
10 Various Doc comment typos Minor

Would you like me to go ahead and fix any of these?