Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0006303 [Squeak] VM minor always 03-05-07 18:59 01-09-08 03:38
Reporter johnmci View Status public  
Assigned To
Priority normal Resolution open  
Status new   Product Version 3.9
Summary 0006303: Matrix2x3Plugin does not consider negative numbers when rounding.
Description roundAndStoreResultPoint: nItemsToPop
    "Store the result of a previous operation.
    Fail if we cannot represent the result as SmallInteger"
    m23ResultX := m23ResultX + 0.5.
    m23ResultY := m23ResultY + 0.5.
Additional Information
Attached Files  M23BugTests-M6303-wiz.1.cs [^] (3,745 bytes) 01-06-08 04:51
 Matrix2x3Plugi...ResultPoint.st [^] (966 bytes) 01-09-08 03:33

- Relationships

SYSTEM WARNING: Creating default object from empty value

related to 0005605assigned andreas Squeak oxymorons: MatrixTransform2x3 identity does not behave as an identiy transformation. 
child of 0006511new  Mother of all Morphic Graphical off-by-one/fencepost -error reports. 

- Notes
(0010380 - 143 - 324 - 324 - 324 - 324 - 324)
johnmci
03-05-07 19:00

rounded
    "Answer the integer nearest the receiver."

    self >= 0.0
        ifTrue: [^(self + 0.5) truncated]
        ifFalse: [^(self - 0.5) truncated]
 
(0011112 - 106 - 106 - 106 - 106 - 106 - 106)
tim
09-12-07 00:28

Reminder sent to: johnmci

Err, care to explain your note about rounded? What class? What relation to any changes in Matrix2x3Plugin?
 
(0011593 - 27 - 27 - 27 - 27 - 27 - 27)
tim
12-27-07 23:10

Reminder sent to: johnmci

Well, any thoughts on this?
 
(0011600 - 130 - 130 - 130 - 130 - 130 - 130)
johnmci
12-28-07 05:04

This became a non-issue in Sophie somewhere, likely the rome migration. Still I think it is a bug? if anyone confirm confirm that.
 
(0011623 - 2054 - 2273 - 2273 - 2375 - 2375 - 2375)
wiz
01-05-08 01:27

I think there is more than one bug here.

The method Matrix2x3Plugin>>roundAndStoreResultPoint:
lacks correctness when dealling with negative numbers.

The two routines that call this:
M23>>primitiveTransformPoint
M23>primitiveInvertPoint
would be prone to cumulative error problems because they are rounding the points (too) early and (too) often.
I suspect they were meant to be used ONLY with screen points scaled up by about 2**15. If used on unscaled points the error would be a significant. Especially on round trips.

I suspect in practice the scaling is not always used.

The store point routine calls
Interpreter>>makePointwithxValue: xValue yValue: yValue
which expects SmallInteger inputs.

The primitives seem incapable of returning points with float coordinates.

Finally screen rectangles will lose shape stability when origin and corner are rounded or truncated. What is wanted is probably floor. That would guarentee they are both shifted in the same direction.

The rounding and truncating are a poor mans floor when everything is assumed to be in the first quadrant. Transformation morphs usually insure this is not true.

See the picture examples in
0002453: [Bugs] What is the extent of a truncated rectangle?


What's needed are Transform and Invert primitives that preserve the precision of the results. Allowing appropriate flooring, compression or expansion to take place at the end of the chain of transformations guided by the user and the context in which the point is used.

I think Andreas originally intended that the chain of transformations take place by composing the M23s and then the final one is used at the end to transform points. Others who are not Andreas have probably writen code without that understanding. Indeed it is such an obsure point I wouldn't be surprised if even Andreas missed it from time to time.

I haven't had time to look for all the possible bugs that tie into this. My impression is there are many.

Yours in curiosity and service, --Jerome Peace
 
(0011628 - 208 - 247 - 247 - 247 - 247 - 247)
nicolas cellier
01-05-08 20:15

I confirm what john said:

-2.7 + 0.5 -> -2.2 -> -2 (once implicitly cast by C from double to int)
-2.7 rounded -> -3

Of course, that would not solve all these early rounding problems Jerome told about.
 
(0011629 - 302 - 350 - 350 - 350 - 350 - 350)
wiz
01-06-08 00:11

I'm a firm believer in partial progress.

Until the deeper problems can be tackled, correcting the rounding of the negative numbers would be an improvement.

That would be a good increment.

And if someone wanted to write an non-casting point maker that would be another good step.

Cheers -Jer
 
(0011631 - 1617 - 1833 - 1961 - 1961 - 1961 - 1961)
wiz
01-06-08 04:58

From the preamble:

Change Set: M23BugTests-wiz
Date: 5 January 2008
Author: (wiz) Jerome Peace

wiz 1/5/2008 23:28

This is the start of tests for MatrixTransform2x3 bugs.
Currently we are testing the bug in roundoff of negative numbers by the m23primitives for transformPoint and invertPoint.

The invariant tested is that the result of calling a transform should be equivalent to the result that would be given if the primitive fails.

Also currently the only suspect being tested is the identity transform.

This fails because of the bug mentioned in mantis 6303
http://bugs.squeak.org/view.php?id=6303. [^]

To do: a test for the Matrix plugin needs to be made. The test will trivially pass if the module is absent.

The list of suspects will need to be widened once a repair makes it pass the identiy case.

Other notes: I have crafted the tests so that upon failure 'rejects explore' will give infomation as to which suspects failed and with which test points.

Doing this currently shows the one suspect fails for all points containing at least one negative coordinate.

====

Widening the list of suspects shows that some of the scaling transformation can pass the tests. This makes sense because scaling by a large enough amount makes the roundoff error less important.

Anyway the routine is a good initial test which can be improved.

I am currently running on my old iMac under 9.1 using a classic os vm. So unless John goes back to supporting that platform I won't be the first one to see this test turn green.

Hth,

Yours in curiosity and service, --Jerome Peace
 
(0011644 - 324 - 360 - 360 - 360 - 360 - 360)
wiz
01-09-08 03:38

Matrix2x3Plugi...ResultPoint.st uploaded.

This is a swing at just the specific patch that could make the test pass.
I say could because I am not set up to test it or even use it. ( My mac has no facility for compiling vm's or c code and I am not experienced there (yet).
Consider this a starting point.

Cheers, -Jer
 

- Issue History
Date Modified Username Field Change
03-05-07 18:59 johnmci New Issue
03-05-07 19:00 johnmci Note Added: 0010380
03-07-07 06:52 wiz Relationship added related to 0005605
05-25-07 02:48 wiz Relationship added related to 0006511
05-25-07 03:00 wiz Relationship replaced child of 0006511
09-12-07 00:28 tim Note Added: 0011112
12-27-07 23:10 tim Note Added: 0011593
12-28-07 05:04 johnmci Note Added: 0011600
01-05-08 01:27 wiz Note Added: 0011623
01-05-08 20:15 nicolas cellier Note Added: 0011628
01-06-08 00:11 wiz Note Added: 0011629
01-06-08 04:51 wiz File Added: M23BugTests-M6303-wiz.1.cs
01-06-08 04:58 wiz Note Added: 0011631
01-09-08 03:33 wiz File Added: Matrix2x3Plugi...ResultPoint.st
01-09-08 03:38 wiz Note Added: 0011644


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
83 total queries executed.
51 unique queries executed.
Powered by Mantis Bugtracker