bugGnash - The GNU Flash player - Bugs: bug #24280, regression: mario.swf

 
 

bug #24280: regression: mario.swf

Submitter:  Sandro Santilli <strk>
Submitted:  Tue 16 Sep 2008 07:56:49 PM UTC
   
 
Category:  None Severity:  5 - Blocker
Release:  None Status:  Fixed
Privacy:  Public Assigned to:  strk
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 30 Sep 2008 10:21:14 PM UTC, comment #30: 

Added the additional test.
Re-checked mario, snowy, golf...
Matrices seems rock solid now ;)

[ except accuracy ]

Sandro Santilli <strk>
Group Member
Tue 30 Sep 2008 09:26:36 PM UTC, comment #29: 

Test crafted, get_parent hack dropped.
Need more failing tests now, in particular for setting rotation
while _xscale is negative ?

Sandro Santilli <strk>
Group Member
Tue 30 Sep 2008 06:08:00 PM UTC, comment #28: 

I added more roundin in matrix_test.swf so we don't have any expected failure there now. Next step would be to try at
crafting a failing test following the event flow found in MovieClip.as (the test failing if we drop the get_parent hack).

Anyway, the main topic of this bug (mario.swf regression)
is fixed for me, and I confirm snowy.swf is also fixed.

Sandro Santilli <strk>
Group Member
Tue 30 Sep 2008 04:26:50 PM UTC, comment #27: 

_yscale and _xscale are always positive to begin with, even when the matrix y or x components are negative. A change in _xscale sign then means a change in x components sign, but whether they match or not depend on the original sign of the matrix.

I've changed the character.cpp code to switch matrix sign only when _xscale or _yscale change sign. This fixes snowy and mario and recently added tests.

Rotation still has what looks like an incorrect hack (get_parent()), but tests fail if it's removed. Might be an easy fix. No idea.

Benjamin Wolsey <bwy>
Group Member
Mon 29 Sep 2008 08:35:48 PM UTC, comment #26: 

The attached patch seems to give all successes in matrix_test.swf except for accuracy related stuff, but fails in MovieClip.as
when setting scales back to positive after having set them to negative. Particularly interesting this case:

check_equals(_root.transform.matrix.toString(), "(a=1, b=0, c=0, d=-2, tx=0, ty=0)");
_root._yscale = 100;
check_equals(_root.transform.matrix.toString(), "(a=1, b=0, c=0, d=1, tx=0, ty=0)");


The first check succeeds (so you know what input is).
The second check fails in Gnash (but only if we change character::set_y_scale to use matrix::set_y_scale rather then ::set_scale_rotation

The xpasses.patch changes matrix_test.c to expect the successes.

(file #16587, file #16588)

Sandro Santilli <strk>
Group Member
Mon 22 Sep 2008 07:45:48 AM UTC, comment #25: 

With the new matrix math, and using no caches, we get many
XPASS in matrix_test.swf and some FAIL in matrix_test.swf
and MovieClip-v*.swf.

The failures in matrix_test.swf are interesting.
Look at this:

FAIL: matrix_testrunner: expected: '(a=-2.23, b=-0.08, c=0.08, d=-2.23, tx=200, ty=200)' obtained: (a=2.23, b=0.08, c=-0.08, d=2.23, tx=200, ty=200) [/home/strk/src/gnash/gnash-head/testsuite/misc-ming.all/matrix_test.c:1389]

Altought the matrix values are different, further getBounds
is correct:

PASSED: printBounds(staticmc.getBounds(_root), 0)  ==  '131,131 270,270' [/home/strk/src/gnash/gnash-head/testsuite/misc-ming.all/matrix_test.c:1393]



Sandro Santilli <strk>
Group Member
Fri 19 Sep 2008 04:25:59 PM UTC, comment #24: 

This is the first interesting spot:

Original matrix:
|  -1.0000    2.0000  200.0000 |
|   0.0000    1.0000  200.0000 |

After setting _yscale = -100:

|  -1.0000 (a)   0.0000 (c) 200.0000 (tx) |
|   0.0000 (b)  -2.2361 (d) 200.0000 (ty) |

 expected: '(a=-1, b=0, c=-2, d=-1, tx=200, ty=200)'


Sandro Santilli <strk>
Group Member
Fri 19 Sep 2008 02:32:35 PM UTC, comment #23: 

Forget about the plan now.
News are that it is possible to inspect matrix values
from ActionScript !!
So I updated matrix_test.swf with that and shows what
the values of the matrix should be when setting parameters.
Looks great to me, should be much easier now to get it right.

Benjamin is working on adding the ActionScript support
side of things.

Sandro Santilli <strk>
Group Member
Fri 19 Sep 2008 06:47:57 AM UTC, comment #22: 

Ok. I have a plan.
The talk with lesshaste wasn't unhelpful after all.

I think for gnash::matrix, setters for rotation and scaling
methods just do not make sense.

Parameters set for display objects should express the amount
of scaling and rotation to apply to the original transformation,
so we need the original transformation around !

That said, the plan is to maintan the original matrix
just the way it is, and apply scaling and rotation
as kept by the caches to it everytime they change.

This means taht any setting will NOT be destructive of
the original thing.

I'll try this approach and let you know.

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 04:10:23 PM UTC, comment #21: 

So, the new version of matrix_test.swf adds a green square
on the middle-right side of the red square so you can
tell orientation of it.
Also, when you click the movie stops/resumes.

It seems to me that the gnash renderer isn't interpreting
all the matrices in the correct way, but maybe it's
the exact same problem arising when setting _yscale
as it seems to happen exactly where XFAIL starts to appear.

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 01:59:18 PM UTC, comment #20: 

FYI: lesshaste on #gnash is saying it should be possible for getters and setters to be symmetric, would you mind joining to try at figuring that out ?

If it is possible, we want to do that rather then
expecting odd results in libcore.all/MatrixTest.cpp

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 01:50:35 PM UTC, comment #19: 

(1) the matrix itself should be fixed.  eg. ::get_rotation() should extract the correct value from the swf tag defined matrix.

(2) cache should be a separate problem which could be fixed at a higher level(in character class).

I suspect the matrix class is indeed not stable, but surely not a accuracy problem.  The determinant() thing might be dropped...

I'll work on it this weekend. The matrix class need a carefull check and we already have many tests...

Zou Lunkai <zoulunkai>
Thu 18 Sep 2008 11:19:46 AM UTC, comment #18: 

I tested golf.swf myself. With the patch applied it goes
back to be turned backward (that old bug).

This time we have debugging though:
 ERROR: Matrix not invertible, setting to identity on invert request

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 10:55:32 AM UTC, comment #17: 

The patch is attached, for broader testing.
I confirm it fixes mario.swf and snowy.swf for me, and breaks
82 tests in matrix_test.swf w/out any xpass.

Benjamin, can you try reproducing the golf.swf bug with this
and run any other interactive movie out there with this applied ?

(file #16517)

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 10:15:09 AM UTC, comment #16: 


> Both snowy.swf and mario.swf behave correctly by applying
> your patch to matrix::get_x_scale()
> and making use of determinant() in matrix::get_rotation().


applying the patch and using determinant() in matrix::get_rotation() recovers all the math in 0.8.3.  Now we may want to inspect the testsuite and add more tests from this “correct” point.





Zou Lunkai <zoulunkai>
Thu 18 Sep 2008 09:48:59 AM UTC, comment #15: 

Both snowy.swf and mario.swf behave correctly by applying
your patch to matrix::get_x_scale()
and making use of determinant() in matrix::get_rotation().

On the other end, extraction of matrix components from
SWF-defined matrix (matrix_test.c) gives lots more failures.

I might try adding additional methods in matrix:

 matrix::get_y_scale_component()
 matrix::get_x_scale_component()
 matrix::get_rotation_component()

The new methods could be used for extraction of parameters
while the old methods (modified using your patch) could
be used by renderer.

Still, current trunk uses matrix::set_x_scale on setting _xscale
rather then ::set_scale_rotation as that one fixed premature death of Mario, so we likely also need to inspect setting
by components..

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 09:12:31 AM UTC, comment #14: 

a small patch for verification.

// old code:
 
double
matrix::get_x_scale() const
{
    return sqrt(((double)sx sx + (double)shx shx)) / 65536.0;
}
 
 
// new code:
 
double
matrix::get_x_scale() const
{
    double x = sqrt(((double)sx sx + (double)shx shx)) / 65536.0;
 
    if( determinant() < 0 )
    {
        x = -x;
    }
    return x;
}

That's the only math change I did after 0.8.3.  I dropped determinant() check because the comments in the old code was not clear about whether it was correct. And get_y_scale() does not do the same check strangely(It's actually turned out to be not strange after the discussion on the gnash-dev mail-list).

This patch made mario.swf works correct here with out using caches. you may want to try it.


Zou Lunkai <zoulunkai>
Thu 18 Sep 2008 08:39:41 AM UTC, comment #13: 

Zou confirms the bug at time of early introduction of int-based matrix (the backward walks in mario.swf)

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 06:41:29 AM UTC, comment #12: 

Another try: don't modify matrix::get_rotation (was modified in your patch) but still don't use parameters caching.
Results: mario.swf is ok. snowy.swf still flips while moving to the left.

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 06:32:27 AM UTC, comment #11: 

Ok, modified your patch to properly call ::set_matrix
instead of directly modifying the matrix (needed for inv.bounds)
and to use a preprocessor macro to switch between cached
and non-cached matrix version.

Still, mario.swf doesn't play as correctly as it did in 0.8.3.
In particular mario and some enemies end up walking backward.

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 06:13:45 AM UTC, comment #10: 

The patch basically drops the matrix cache. We may use a compile-time switch and play a bit w/out it.
If all it breaks is our own tests well, I'd say let's ship
w/out caches...

Sandro Santilli <strk>
Group Member
Thu 18 Sep 2008 02:25:14 AM UTC, comment #9: 

So as far as I can tell there are some circumstances in which the direction of rotation (clockwise or counterclockwise) is particularly relevant. Given all the testsuite failures the attached patch generates, that rotation implementation is almost certainly incorrect. Nevertheless, it seems to render Mario.swf and snowy.swf correctly. So this might help a bit.

(file #16512)

Bastiaan Jacques <bjacques>
Group Member
Wed 17 Sep 2008 04:55:33 PM UTC, comment #8: 

added zoulunkai in Cc list, he being the one having spent
more time on the matrix math...

Sandro Santilli <strk>
Group Member
Wed 17 Sep 2008 04:24:35 PM UTC, comment #7: 

This game shows a similar regression:
http://foo.keybit.net/~strk/tmp/snowy.swf

The character walks backward (but only when going left).
Seems kind of unable to turn.
Also in this case the game plaied fine in 0.8.3.

Sandro Santilli <strk>
Group Member
Wed 17 Sep 2008 03:55:32 PM UTC, comment #6: 

Added tests for _yscale setting as well.
The use of matrix::set_y_scale in its current state didn't help though. Nor did using the determinant() trick for ::set_y_scale only.

Sandro Santilli <strk>
Group Member
Wed 17 Sep 2008 03:44:12 PM UTC, comment #5: 

Added tests in matrix_test.c for setting _xscale.
Interestingly enough getBoundaries returns the same
boundaries with _xscale changing sign
(initial sign read from SWF, further changed with AS).
The new tests are fixed if we use matrix::set_x_scale
only passing the new _xscale rather then recomputing
tha matrix from the caches.

Will add similar tests for _yscale next.

Sandro Santilli <strk>
Group Member
Wed 17 Sep 2008 12:40:43 PM UTC, comment #4: 

Changing character::set_x_scale to use matrix::set_x_scale
rather then ::set_scale_rotation also partially fixes
the issue.
I guess next step could be adding more tests into matrix_test.c
to try effects of setting _xscale, _yscale and _rotation trought actionscript to getBounds and hitTest...

Sandro Santilli <strk>
Group Member
Wed 17 Sep 2008 09:35:23 AM UTC, comment #3: 

To reduce scope of the bug, it can be verified even w/out user interaction. Just look at the enemies walking on second floor.
The one reaching the end of the floor will get upside down when coming back. Again, this worked fine in 0.8.3.

Sandro Santilli <strk>
Group Member
Wed 17 Sep 2008 07:26:00 AM UTC, comment #2: 

Reverting the change to matrix::get_rotation fixes mario dying but makes it walk backwards.
0.8.3 worked correctly.

Sandro Santilli <strk>
Group Member
Wed 17 Sep 2008 06:35:44 AM UTC, comment #1: 

Revision 9642.1.2 (strk@keybit.net-20080826122638-gto7d43cofcvsagg)
introduced the bug.

This was a change in gnash::matrix::get_rotation() dropping
the determinant() test. The change fixed all tests in misc-ming.all/matrix_text.c, but it seems it broke mario.swf.

Both mario.swf and matrix_test.swf are SWF6.

Sandro Santilli <strk>
Group Member
Tue 16 Sep 2008 07:56:49 PM UTC, original submission:  

http://foo.keybit.net/~strk/tmp/mario.swf

In 0.8.3 you could move mario around.
With trunk, he dies as soon as you go left.

Sandro Santilli <strk>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #16587:  noScaleRotation.patch added by strk (695B - text/x-patch)
file #16588:  xpasses.patch added by strk (30KiB - text/x-patch)
file #16517:  matrix.patch added by strk (656B - application/octet-stream)
file #16512:  rotation.diff added by bjacques (4KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by bwy (Posted a comment)
  • -email is unavailable- added by zoulunkai (Posted a comment)
  • -email is unavailable- added by bjacques (Updated the item)
  • -email is unavailable- added by strk
  • -email is unavailable- added by strk (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-09-30 strk Open/ClosedOpen Closed
    2008-09-30 strk StatusReady For Test Fixed
    2008-09-30 strk StatusIn Progress Ready For Test
    2008-09-29 strk Attached File- Added noScaleRotation.patch, #16587
        Attached File- Added xpasses.patch, #16588
    2008-09-18 strk Attached File- Added matrix.patch, #16517
    2008-09-18 bjacques Attached File- Added rotation.diff, #16512
    2008-09-17 strk Carbon-Copy- Added zoulunkai
    2008-09-17 strk Assigned toNone strk
    2008-09-17 strk StatusNone In Progress

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code