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

 
 

bug #24280: regression: mario.swf

Submitted by:  Sandro Santilli <strk>
Submitted on:  Tue 16 Sep 2008 07:56:49 PM UTC  
 
Category: NoneSeverity: 5 - Blocker
Release: NoneStatus: Fixed
Privacy: PublicAssigned to: Sandro Santilli <strk>
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.

 

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

Attach File(s):
   
   
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
  • -unavailable- added by bwy (Posted a comment)
  • -unavailable- added by zoulunkai (Posted a comment)
  • -unavailable- added by bjacques (Updated the item)
  • -unavailable- added by strk
  • -unavailable- added by strk (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 30 Sep 2008 10:21:14 PM UTCstrkOpen/ClosedOpen=>Closed
    Tue 30 Sep 2008 10:21:13 PM UTCstrkStatusReady For Test=>Fixed
    Tue 30 Sep 2008 06:08:00 PM UTCstrkStatusIn Progress=>Ready For Test
    Mon 29 Sep 2008 08:35:48 PM UTCstrkAttached File-=>Added noScaleRotation.patch, #16587
      Attached File-=>Added xpasses.patch, #16588
    Thu 18 Sep 2008 10:55:32 AM UTCstrkAttached File-=>Added matrix.patch, #16517
    Thu 18 Sep 2008 02:25:13 AM UTCbjacquesAttached File-=>Added rotation.diff, #16512
    Wed 17 Sep 2008 04:55:33 PM UTCstrkCarbon-Copy-=>Added zoulunkai
    Wed 17 Sep 2008 06:35:44 AM UTCstrkAssigned toNone=>strk
    Wed 17 Sep 2008 06:35:43 AM UTCstrkStatusNone=>In Progress

    Back to the top


    Powered by Savane 3.1-cleanup1