From 1cc22ae2aad7b2e8c8794655ebd2d5ea7165bbfe Mon Sep 17 00:00:00 2001 From: zeromus Date: Mon, 20 Feb 2017 23:22:54 -0600 Subject: [PATCH] fix matrix stack error conditions and handling (fixes #39) (savestates are broken) --- desmume/src/gfx3d.cpp | 164 +++++++++++++++++++++++++---------------- desmume/src/matrix.cpp | 42 +---------- desmume/src/matrix.h | 6 +- 3 files changed, 108 insertions(+), 104 deletions(-) diff --git a/desmume/src/gfx3d.cpp b/desmume/src/gfx3d.cpp index 09b6032e0..57f843517 100644 --- a/desmume/src/gfx3d.cpp +++ b/desmume/src/gfx3d.cpp @@ -1,6 +1,6 @@ /* Copyright (C) 2006 yopyop - Copyright (C) 2008-2016 DeSmuME team + Copyright (C) 2008-2017 DeSmuME team This file is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -28,6 +28,13 @@ //if they do, then we need to copy them out in doFlush!!! //--------------- +//old tests of matrix stack: +//without the mymode==(texture) namco classics galaxian will try to use pos=1 and overrun the stack, corrupting emu +//according to gbatek, 31 works but sets the stack overflow flag. spider-man 2 tests this on the spiderman model (and elsewhere) +//see "Tony Hawk's American Sk8land" work from revision 07806eb8364d1eb2d21dd3fe5234c9abe0a8a894 +//from MatrixStackSetStackPosition: "once upon a time, we tried clamping to the size. this utterly broke sims 2 apartment pets. changing to wrap around made it work perfectly" +//Water Horse Legend Of The Deep does seem to exercise the texture matrix stack, which does probably need to exist *but I'm not 100% sure) + #include "gfx3d.h" #include @@ -282,10 +289,11 @@ static FragmentColor *_gfx3d_colorRGBA6665 = NULL; static u16 *_gfx3d_colorRGBA5551 = NULL; // Matrix stack handling +//TODO: decouple stack pointers from matrix stack type CACHE_ALIGN MatrixStack mtxStack[4] = { MatrixStack(1, 0), // Projection stack - MatrixStack(31, 1), // Coordinate stack - MatrixStack(31, 2), // Directional stack + MatrixStack(32, 1), // Coordinate stack + MatrixStack(32, 2), // Directional stack MatrixStack(1, 3), // Texture stack }; @@ -577,7 +585,6 @@ void gfx3d_reset() MatrixStackInit(&mtxStack[0]); MatrixStackInit(&mtxStack[1]); MatrixStackInit(&mtxStack[2]); - MatrixStackInit(&mtxStack[3]); clCmd = 0; clInd = 0; @@ -936,95 +943,128 @@ static void gfx3d_glMatrixMode(u32 v) static void gfx3d_glPushMatrix() { - //this command always works on both pos and vector when either pos or pos-vector are the current mtx mode - const MatrixMode mymode = ((mode == MATRIXMODE_POSITION) ? MATRIXMODE_POSITION_VECTOR : mode); +//in case we're in Position-Vector mode, actually work on the Position matrix first (we'll update the vector matrix secondarily) + const MatrixMode mymode = ((mode == MATRIXMODE_POSITION_VECTOR) ? MATRIXMODE_POSITION : mode); - MatrixStackPushMatrix(&mtxStack[mymode], mtxCurrent[mymode]); + //1. apply offset specified by push (well, it's always +1) to internal counter + //2. mask that bit off to actually index the matrix for reading + //3. SE is set depending on resulting internal counter + + if(mymode == MATRIXMODE_PROJECTION || mymode == MATRIXMODE_TEXTURE) + { + u32& index = mtxStack[mymode].position; + MatrixCopy(MatrixStackGetPos(&mtxStack[mymode], index&1), mtxCurrent[mymode]); + + index += 1; + index &= 3; + if(index >= 2) MMU_new.gxstat.se = 1; //unknown if this applies to the texture matrix + } + else + { + u32& index = mtxStack[MATRIXMODE_POSITION].position; + MatrixCopy(MatrixStackGetPos(&mtxStack[MATRIXMODE_POSITION], index&31), mtxCurrent[MATRIXMODE_POSITION]); + + if(mode == MATRIXMODE_POSITION_VECTOR) + MatrixCopy(MatrixStackGetPos(&mtxStack[MATRIXMODE_POSITION_VECTOR], index&31), mtxCurrent[MATRIXMODE_POSITION_VECTOR]); + + index += 1; + index &= 63; + if(index >= 32) MMU_new.gxstat.se = 1; + } GFX_DELAY(17); - - if (mymode == MATRIXMODE_POSITION_VECTOR) - MatrixStackPushMatrix(&mtxStack[1], mtxCurrent[1]); } -static void gfx3d_glPopMatrix(s32 i) +static void gfx3d_glPopMatrix(s32 v) { - // The stack has only one level (at address 0) in projection mode, - // in that mode, the parameter value is ignored, the offset is always +1 in that mode. - if (mode == MATRIXMODE_PROJECTION) i = 1; + //in case we're in Position-Vector mode, actually work on the Position matrix first (we'll update the vector matrix secondarily) + const MatrixMode mymode = ((mode == MATRIXMODE_POSITION_VECTOR) ? MATRIXMODE_POSITION : mode); - //this command always works on both pos and vector when either pos or pos-vector are the current mtx mode - const MatrixMode mymode = ((mode == MATRIXMODE_POSITION) ? MATRIXMODE_POSITION_VECTOR : mode); + //1. apply offset specified by pop to internal counter + //2. SE is set depending on resulting internal counter + //3. mask that bit off to actually index the matrix for reading - //i = (i<<26)>>26; - //previously, we sign extended here. that isnt really necessary since the stacks are apparently modularly addressed. so i am somewhat doubtful that this is a real concept. - //example: - //suppose we had a -30 that would be %100010. - //which is the same as adding 34. if our stack was at 17 then one way is 17-30(+32)=19 and the other way is 17+34(-32)=19 - - //please note that our ability to skip treating this as signed is dependent on the modular addressing later. if that ever changes, we need to change this back. + if(mymode == MATRIXMODE_PROJECTION || mymode == MATRIXMODE_TEXTURE) + { + //parameter ignored and treated as sensible, as a pop argument anyway + v = 1; + + u32& index = mtxStack[mymode].position; - MatrixStackPopMatrix(mtxCurrent[mymode], &mtxStack[mymode], i); + index -= v; + index &= 3; + if(index >= 2) MMU_new.gxstat.se = 1; //unknown if this applies to the texture matrix + + MatrixCopy(mtxCurrent[mymode], MatrixStackGetPos(&mtxStack[mymode], index&1)); + } + else + { + u32& index = mtxStack[MATRIXMODE_POSITION].position; + + index -= v; + index &= 63; + if(index >= 32) MMU_new.gxstat.se = 1; + + MatrixCopy(mtxCurrent[MATRIXMODE_POSITION], MatrixStackGetPos(&mtxStack[MATRIXMODE_POSITION], index&31)); + + if(mode == MATRIXMODE_POSITION_VECTOR) + MatrixCopy(mtxCurrent[MATRIXMODE_POSITION_VECTOR], MatrixStackGetPos(&mtxStack[MATRIXMODE_POSITION_VECTOR], index&31)); + } GFX_DELAY(36); - - if (mymode == MATRIXMODE_POSITION_VECTOR) - MatrixStackPopMatrix(mtxCurrent[1], &mtxStack[1], i); } static void gfx3d_glStoreMatrix(u32 v) { - //this command always works on both pos and vector when either pos or pos-vector are the current mtx mode - const MatrixMode mymode = ((mode == MATRIXMODE_POSITION) ? MATRIXMODE_POSITION_VECTOR : mode); + //in case we're in Position-Vector mode, actually work on the Position matrix first (we'll update the vector matrix secondarily) + const MatrixMode mymode = ((mode == MATRIXMODE_POSITION_VECTOR) ? MATRIXMODE_POSITION : mode); - //limit height of these stacks. - //without the mymode==3 namco classics galaxian will try to use pos=1 and overrun the stack, corrupting emu - if (mymode == MATRIXMODE_PROJECTION || mymode == MATRIXMODE_TEXTURE) + if(mymode == MATRIXMODE_PROJECTION || mymode == MATRIXMODE_TEXTURE) + { + //parameter ignored and treated as sensible v = 0; + } + else + { + //out of bounds function fully properly, but set errors + if(v >= 31) MMU_new.gxstat.se = 1; + } + //the counter is [0,63] but only the bottom 5 bits index the stack v &= 31; - //according to gbatek, 31 works but sets the stack overflow flag - //spider-man 2 tests this on the spiderman model (and elsewhere) - //i am somewhat skeptical of this, but we'll leave it this way for now. - //a test shouldnt be too hard - if (v == 31) - MMU_new.gxstat.se = 1; - MatrixStackLoadMatrix(&mtxStack[mymode], v, mtxCurrent[mymode]); - GFX_DELAY(17); + //apply to Position-Vector matrix too, if appropriate + if (mode == MATRIXMODE_POSITION_VECTOR) + MatrixStackLoadMatrix(&mtxStack[MATRIXMODE_POSITION_VECTOR], v, mtxCurrent[MATRIXMODE_POSITION_VECTOR]); - if (mymode == MATRIXMODE_POSITION_VECTOR) - MatrixStackLoadMatrix(&mtxStack[1], v, mtxCurrent[1]); + GFX_DELAY(17); } static void gfx3d_glRestoreMatrix(u32 v) { - //this command always works on both pos and vector when either pos or pos-vector are the current mtx mode - const MatrixMode mymode = ((mode == MATRIXMODE_POSITION) ? MATRIXMODE_POSITION_VECTOR : mode); + //in case we're in Position-Vector mode, actually work on the Position matrix first (we'll update the vector matrix secondarily) + const MatrixMode mymode = ((mode == MATRIXMODE_POSITION_VECTOR) ? MATRIXMODE_POSITION : mode); - //limit height of these stacks - //without the mymode==3 namco classics galaxian will try to use pos=1 and overrun the stack, corrupting emu - if (mymode == MATRIXMODE_PROJECTION || mymode == MATRIXMODE_TEXTURE) + if(mymode == MATRIXMODE_PROJECTION || mymode == MATRIXMODE_TEXTURE) + { + //parameter ignored and treated as sensible v = 0; - - v &= 31; - - //according to gbatek, 31 works but sets the stack overflow flag - //spider-man 2 tests this on the spiderman model (and elsewhere) - //i am somewhat skeptical of this, but we'll leave it this way for now. - //a test shouldnt be too hard - if (v == 31) - MMU_new.gxstat.se = 1; - + } + else + { + //out of bounds errors function fully properly, but set errors + MMU_new.gxstat.se = v>=31; + } MatrixCopy(mtxCurrent[mymode], MatrixStackGetPos(&mtxStack[mymode], v)); - GFX_DELAY(36); + //apply to Position-Vector matrix too, if appropriate + if (mode == MATRIXMODE_POSITION_VECTOR) + MatrixCopy(mtxCurrent[MATRIXMODE_POSITION_VECTOR], MatrixStackGetPos(&mtxStack[MATRIXMODE_POSITION_VECTOR], v)); - if (mymode == MATRIXMODE_POSITION_VECTOR) - MatrixCopy(mtxCurrent[1], MatrixStackGetPos(&mtxStack[1], v)); + GFX_DELAY(36); } static void gfx3d_glLoadIdentity() @@ -2544,7 +2584,7 @@ void gfx3d_savestate(EMUFILE* os) for (size_t i = 0; i < polylist->count; i++) polylist->list[i].save(os); - for (size_t i = 0; i < 4; i++) + for (size_t i = 0; i < ARRAY_SIZE(mtxStack); i++) { OSWRITE(mtxStack[i].position); for(size_t j = 0; j < mtxStack[i].size*16; j++) @@ -2595,7 +2635,7 @@ bool gfx3d_loadstate(EMUFILE* is, int size) if (version >= 2) { - for (size_t i=0; i < 4; i++) + for (size_t i=0; i < ARRAY_SIZE(mtxStack); i++) { OSREAD(mtxStack[i].position); for(size_t j = 0; j < mtxStack[i].size*16; j++) diff --git a/desmume/src/matrix.cpp b/desmume/src/matrix.cpp index 168525ef4..b3c175aae 100644 --- a/desmume/src/matrix.cpp +++ b/desmume/src/matrix.cpp @@ -1,6 +1,6 @@ /* Copyright (C) 2006-2007 shash - Copyright (C) 2007-2012 DeSmuME team + Copyright (C) 2007-2017 DeSmuME team This file is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -216,7 +216,7 @@ void MatrixStackSetMaxSize (MatrixStack *stack, int size) { int i; - stack->size = (size + 1); + stack->size = size; if (stack->matrix != NULL) { free (stack->matrix); @@ -227,8 +227,6 @@ void MatrixStackSetMaxSize (MatrixStack *stack, int size) { MatrixInit (&stack->matrix[i*16]); } - - stack->size--; } @@ -238,42 +236,10 @@ MatrixStack::MatrixStack(int size, int type) this->type = type; } -static void MatrixStackSetStackPosition (MatrixStack *stack, int pos) -{ - stack->position += pos; - - if((stack->position < 0) || (stack->position > stack->size)) - MMU_new.gxstat.se = 1; - - //once upon a time, we tried clamping to the size. - //this utterly broke sims 2 apartment pets. - //changing to wrap around made it work perfectly - stack->position = ((u32)stack->position) & stack->size; -} - -void MatrixStackPushMatrix (MatrixStack *stack, const s32 *ptr) -{ - //printf("Push %i pos %i\n", stack->type, stack->position); - if ((stack->type == 0) || (stack->type == 3)) - MatrixCopy (&stack->matrix[0], ptr); - else - MatrixCopy (&stack->matrix[stack->position*16], ptr); - MatrixStackSetStackPosition (stack, 1); -} - -void MatrixStackPopMatrix (s32 *mtxCurr, MatrixStack *stack, int size) -{ - //printf("Pop %i pos %i (change %d)\n", stack->type, stack->position, -size); - MatrixStackSetStackPosition(stack, -size); - if ((stack->type == 0) || (stack->type == 3)) - MatrixCopy (mtxCurr, &stack->matrix[0]); - else - MatrixCopy (mtxCurr, &stack->matrix[stack->position*16]); -} s32* MatrixStackGetPos(MatrixStack *stack, const size_t pos) { - assert(pos<31); + assert(pos < stack->size); return &stack->matrix[pos*16]; } @@ -284,7 +250,7 @@ s32* MatrixStackGet (MatrixStack *stack) void MatrixStackLoadMatrix (MatrixStack *stack, const size_t pos, const s32 *ptr) { - assert(pos<31); + assert(pos < stack->size); MatrixCopy(&stack->matrix[pos*16], ptr); } diff --git a/desmume/src/matrix.h b/desmume/src/matrix.h index a9de4b9c1..8a1cbc222 100644 --- a/desmume/src/matrix.h +++ b/desmume/src/matrix.h @@ -1,6 +1,6 @@ /* Copyright (C) 2006-2007 shash - Copyright (C) 2007-2012 DeSmuME team + Copyright (C) 2007-2017 DeSmuME team This file is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -37,7 +37,7 @@ struct MatrixStack { MatrixStack(int size, int type); s32 *matrix; - s32 position; + u32 position; s32 size; u8 type; }; @@ -58,8 +58,6 @@ void MatrixIdentity (s32 *matrix); void MatrixStackInit (MatrixStack *stack); void MatrixStackSetMaxSize (MatrixStack *stack, int size); -void MatrixStackPushMatrix (MatrixStack *stack, const s32 *ptr); -void MatrixStackPopMatrix (s32 *mtxCurr, MatrixStack *stack, int size); s32* MatrixStackGetPos (MatrixStack *stack, const size_t pos); s32* MatrixStackGet (MatrixStack *stack); void MatrixStackLoadMatrix (MatrixStack *stack, const size_t pos, const s32 *ptr);