From 85f72b94d21f0d197f8965471b89a58cd59e5e0e Mon Sep 17 00:00:00 2001 From: ItsNoHax Date: Thu, 13 Aug 2026 14:28:03 +0200 Subject: [PATCH] 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. --- ci/tests/src/gum_test.rs | 116 +++++++++++++++++++++++++++++++++++++++ ci/tests/src/main.rs | 2 + psp/src/sys/gum.rs | 5 +- 3 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 ci/tests/src/gum_test.rs diff --git a/ci/tests/src/gum_test.rs b/ci/tests/src/gum_test.rs new file mode 100644 index 00000000..cd1cdb6b --- /dev/null +++ b/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 +} diff --git a/ci/tests/src/main.rs b/ci/tests/src/main.rs index 6fe43503..623d0008 100644 --- a/ci/tests/src/main.rs +++ b/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, diff --git a/psp/src/sys/gum.rs b/psp/src/sys/gum.rs index ca10ccc5..85b15a7e 100644 --- a/psp/src/sys/gum.rs +++ b/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