fix(gum): store into the current slot before advancing in sceGumPushMatrix

`sceGumPushMatrix` advanced `CURRENT_MATRIX` and then stored the VFPU matrix,
so it saved into the slot above the stack pointer. `sceGumPopMatrix` decrements
and then loads, so it reads the slot below the pointer it was left at. The two
never refer to the same slot, and what pop returns is not what push saved.

`CURRENT_MATRIX` names the slot that mirrors the live matrix -- `sceGumMatrixMode`
and `sceGumUpdateMatrix` both write the VFPU matrix to `*CURRENT_MATRIX` -- so
storing before the increment is also the only ordering that works. Saving into
the slot above and having pop read that slot back is self-consistent in
isolation, but `sceGumUpdateMatrix` writes the live matrix to exactly that slot,
and `sceGumDrawArray` calls it, so the first draw after a push would overwrite
what the push saved. This matches PSPSDK's pspgum.c, which stores and then
increments.

The bug hid behind that same sync. `MATRIX_STACK` is zero initialised, so a pop
with nothing beneath it loads an all-zero matrix and collapses everything drawn
afterwards to a point; but any draw performed before the first push syncs the
correct matrix into the slot pop reads, after which push/pop pairs appear to
work. Code that drew before pushing was fine. Code whose first drawing act was
a push -- for example a model whose wheel meshes are ordered before its body
meshes -- lost every mesh after the first pop.

Adds four regression tests to ci/tests covering push/pop, nesting, a matrix
write between push and pop, and that a push leaves the working matrix alone.
Against master the first three fail with (0.0, 0.0, 0.0) against the expected
translation and the suite ends FINAL_FAILURE; with this change all four pass and
the suite ends FINAL_SUCCESS.

Author: ItsNoHax

ci/tests/src/gum_test.rs

@@ -0,0 +1,116 @@
+//! Regression tests for the sceGum matrix stack: a pop restores what its matching push saved.
+//!
+//! Only the translation column is compared, since it is enough to tell the cases apart and reads
+//! better in a failure message than sixteen floats.
+
+use psp::sys::{self, MatrixMode, ScePspFMatrix4, ScePspFVector3, ScePspFVector4};
+use psp::test_runner::TestRunner;
+
+fn zero_matrix() -> ScePspFMatrix4 {
+    let zero = ScePspFVector4 {
+        x: 0.0,
+        y: 0.0,
+        z: 0.0,
+        w: 0.0,
+    };
+    ScePspFMatrix4 {
+        x: zero,
+        y: zero,
+        z: zero,
+        w: zero,
+    }
+}
+
+fn translation() -> (f32, f32, f32) {
+    let mut m = zero_matrix();
+    unsafe { sys::sceGumStoreMatrix(&mut m) };
+    (m.w.x, m.w.y, m.w.z)
+}
+
+fn translate(x: f32, y: f32, z: f32) {
+    unsafe { sys::sceGumTranslate(&ScePspFVector3 { x, y, z }) };
+}
+
+/// The leading `sceGumLoadIdentity` is not redundant: it and `sceGumLoadMatrix` are the only entry
+/// points that create the VFPU context, and `sceGumMatrixMode` on a cold one traps. See #189.
+fn reset() {
+    unsafe {
+        sys::sceGumLoadIdentity();
+        sys::sceGumMatrixMode(MatrixMode::Model);
+        sys::sceGumLoadIdentity();
+    }
+}
+
+pub fn test_main(test_runner: &mut TestRunner) {
+    test_runner.check_list(&[
+        (
+            "gum_push_pop_restores_translation",
+            push_pop_restores(),
+            (1.0, 2.0, 3.0),
+        ),
+        (
+            "gum_push_pop_nested_restores_translation",
+            nested_push_pop_restores(),
+            (1.0, 2.0, 3.0),
+        ),
+        (
+            "gum_push_pop_survives_matrix_write",
+            push_pop_survives_matrix_write(),
+            (1.0, 2.0, 3.0),
+        ),
+        (
+            "gum_push_leaves_current_matrix_alone",
+            push_does_not_disturb_current(),
+            (1.0, 2.0, 3.0),
+        ),
+    ]);
+}
+
+fn push_pop_restores() -> (f32, f32, f32) {
+    reset();
+    translate(1.0, 2.0, 3.0);
+    unsafe { sys::sceGumPushMatrix() };
+    translate(10.0, 20.0, 30.0);
+    unsafe { sys::sceGumPopMatrix() };
+    translation()
+}
+
+/// Two deep, so a stack that is off by one in either direction is caught.
+fn nested_push_pop_restores() -> (f32, f32, f32) {
+    reset();
+    translate(1.0, 2.0, 3.0);
+    unsafe { sys::sceGumPushMatrix() };
+    translate(10.0, 20.0, 30.0);
+    unsafe { sys::sceGumPushMatrix() };
+    translate(100.0, 200.0, 300.0);
+    unsafe { sys::sceGumPopMatrix() };
+    unsafe { sys::sceGumPopMatrix() };
+    translation()
+}
+
+/// A sync to `*CURRENT_MATRIX` between push and pop must not eat the saved copy. This is what a
+/// draw does, and it rules out saving into the slot above the stack pointer. A mode switch stands
+/// in for `sceGumUpdateMatrix`, which stores through the same instructions but ends in
+/// `sceGuSetMatrix`, and this suite never brings the GU up.
+fn push_pop_survives_matrix_write() -> (f32, f32, f32) {
+    reset();
+    translate(1.0, 2.0, 3.0);
+    unsafe { sys::sceGumPushMatrix() };
+    translate(10.0, 20.0, 30.0);
+    unsafe {
+        sys::sceGumMatrixMode(MatrixMode::View);
+        sys::sceGumMatrixMode(MatrixMode::Model);
+        sys::sceGumPopMatrix();
+    }
+    translation()
+}
+
+/// Pushing saves the matrix without disturbing it.
+fn push_does_not_disturb_current() -> (f32, f32, f32) {
+    reset();
+    translate(1.0, 2.0, 3.0);
+    unsafe { sys::sceGumPushMatrix() };
+    let after_push = translation();
+    unsafe { sys::sceGumPopMatrix() };
+    after_push
+}

ci/tests/src/main.rs

@@ -8,6 +8,7 @@ extern crate alloc;
 use psp::test_runner::TestRunner;
 
 mod bmp_screenshot_test;
+mod gum_test;
 mod math_test;
 mod vfpu_test;
 mod vram_test;
@@ -17,6 +18,7 @@ psp::module!("ci_tests", 1, 1);
 fn psp_main() {
     let tests = &[
         bmp_screenshot_test::test_main,
+        gum_test::test_main,
         math_test::test_main,
         vfpu_test::test_main,
         vram_test::test_main,

psp/src/sys/gum.rs

@@ -458,7 +458,6 @@ pub unsafe extern "C" fn sceGumPopMatrix() {
 #[allow(non_snake_case)]
 #[no_mangle]
 pub unsafe extern "C" fn sceGumPushMatrix() {
-    CURRENT_MATRIX = CURRENT_MATRIX.offset(1);
     get_context_unchecked().prepare(MatrixSet::VMAT3, MatrixSet::empty());
 
     vfpu_asm!(
@@ -469,6 +468,10 @@ pub unsafe extern "C" fn sceGumPushMatrix() {
         in(reg) CURRENT_MATRIX,
         options(nostack),
     );
+
+    // Advance only after the store: `CURRENT_MATRIX` mirrors the live matrix, so the saved copy has
+    // to land in the slot below it, which is where `sceGumPopMatrix` looks.
+    CURRENT_MATRIX = CURRENT_MATRIX.offset(1);
 }
 
 /// Rotate around the X axis