Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0007635 [Squeak] Graphics minor always 05-21-11 04:24 06-06-11 11:03
Reporter wiz View Status public  
Assigned To andreas
Priority normal Resolution won't fix  
Status closed   Product Version trunk
Summary 0007635: Graphics primitives will raise errors when rectangles have widths in fractions.
Description For this one in a work space evaluate

box :=
RectangleMorph new openCenteredInWorld .

box bounds: ((375@280 corner: 425@320) insetBy: (3/4)).

an error box will come up (see attached log).

The alternate form:
box bounds: ((375@280 corner: 425@320) insetBy: (0.75))

will work without error.
Additional Information This is a darn serious bug. Most of all it shouldn't even be a bug since the floating point form of the fraction works just fine.

The problem is in copybits and it has been there a long time.

The fractional form should work like the floating point form. The problem should be taken care of in copy bits.

There are over a hundred and twenty-five methods that call copybits. My guess is many of them choose to work imprecisely because copybits isn't doing its job properly.
Attached Files  SqueakDebug200105202345.log [^] (10,476 bytes) 05-21-11 04:24
 copyBitsFix-wiz.4.cs [^] (4,375 bytes) 05-23-11 17:52
 copyBitsFix-wiz.6.cs [^] (6,275 bytes) 05-23-11 19:27
 copyBitsFix-wiz.8.cs [^] (10,222 bytes) 05-24-11 00:12
 copyBitsFix-wiz.11.cs [^] (10,332 bytes) 05-24-11 00:47

- Relationships

SYSTEM WARNING: Creating default object from empty value

SYSTEM WARNING: Creating default object from empty value

related to 0006486closed laza In 7097 Resizing the objects flaptab can result in a infinite loop (i.e. squeak freezes) 
related to 0007213closed laza [big bug] morph>>extent: now does rounding and that can lead to infinite loops. 

- Notes
(0014113 - 829 - 871 - 1071 - 1071 - 1071 - 1071)
wiz
05-21-11 04:35

The other problem I ran into was that when I choose to abandon the error the system crashed. Apparently this error does not get caught properly by drawSystemSafely. My guess is that it is trying to draw an error box with the same bounds in fractions. I had to deal with the emergency evaluator and eventually kill the job from the system.

Proceeding from the error does correct things by doing some brute force rounding. This is not a correct fix since it depends on the user knowing to press an unexpected button. And the problem is not with the non integer nature of the bound but by the fact they are expressed as a Fraction class object.

This report is a response to this thread:
http://lists.squeakfoundation.org/pipermail/squeak-dev/2011-May/160027.html [^]

Also look for other threads whose title references [M7635]
 
(0014114 - 85 - 85 - 85 - 85 - 85 - 85)
lewis
05-22-11 17:56

This is not broken in Squeak 3.9, so the problem has been introduced since that time.
 
(0014115 - 787 - 831 - 831 - 831 - 831 - 831)
lewis
05-22-11 20:15

On further investigation, in Squeak 3.9 a morph with fractions in its bounds will cause a crash similar to what is now seen in Squeak trunk. In both cases, the problem is associated with the error handling in BitBlt>>copyBits, which does a "self error: 'Bad BitBlt arg (Fraction?); proceed to convert.'" to allow the user to decide if the arguments should be automatically corrected, which works poorly if the #copyBits happens to be running in the morphic update loop.

It is debatable whether the "self error" should be removed, but in any case it appears that Squeak trunk is allowing the bounds for a morph to include (invalid) fractions, whereas this was not the case in earlier Squeak. The problem appears to be in Morph>>extent: which no longer sends #rounded to the new extent.
 
(0014116 - 32 - 32 - 32 - 32 - 32 - 32)
lewis
05-22-11 20:30

Fixed in trunk (Morphic-dtl.542)
 
(0014118 - 2527 - 2647 - 2647 - 2647 - 2647 - 2647)
wiz
05-22-11 21:48

Hi Dave,

I looked at this at the low level of copyBit. And I am of a differing opinion on what the fix should be.

If you continually round things at a high level then you will get cumulative roundoff errors. Morph no longer rounding its rectangles is a step in the direction to fix this. Because of transformation morphs a fractional extent for a morph pre-transformation makes good sense.

Also because of transformation morphs the corners of a morph may be, probably will be, in different quadrants of your coordinate system. Rounding towards zero will distort the shape of the morph and change its aspect ratio against the users wishes. Screen gribbles have been documented to have been caused by this.
----
copyBits tries a lot of different fixes and calls itself recursively to see if they work. At the end it sends an error message and hints one should proceed to round things off. In which case it round things off and calls copyBitsAgain which calls the primitive but then exits to primitive error if that doesn't work. My assumption is that it works because I tried proceeding and don't remember getting another error.

The rounding routine takes every parameter send them each an asInteger which truncates every number towards zero and to put icing on the cake then does a range check to insure each number is within the range of a small integer. This is not quite correct. First all top lefts want to be floored. Widths and Heights I believe want to reach for their ceilings and be insured of being at least 1.

The worst error in copyBits is the calling of #error: itself. Presumably the debugging routines depend on methods that call copyBits. When a graphic will not display correctly reporting an error will cause the screen to be redrawn and the error will be encountered again.

Instead copyBits needs to always succeed even if this means rendering some graphics invisible. Logging errors to memory for later retrieval might be helpful but an error message could crash the system with and infinite recursion.

Therefore if at first it doesn't succeed copyBits should just take the responsibility of checking its parameters carefully and correcting its copy of them not. Morphs should not have the responsibility to work around copyBits limitations.

I am still thinking about the best way to correct copyBits. Please revert Morphic-dtl.542 back to the previous method. It is the wrong level at which to fix this bug.

Yours in curiosity and service, --Jerome Peace, fearless bug tracker
 
(0014119 - 74 - 74 - 74 - 74 - 74 - 74)
wiz
05-22-11 21:51

see previous note problem needs to be fixed at the level of copyBits --wiz
 
(0014120 - 241 - 247 - 247 - 247 - 247 - 247)
lewis
05-22-11 22:20

The fix applied in Morphic-dtl.542 restores the original behavior of Morph>>extent:, which had been lost in an update to trunk about a year ago. I cannot say if other changes would be warranted, probably it's better to discuss on squeak-dev.
 
(0014121 - 529 - 565 - 565 - 565 - 565 - 565)
wiz
05-23-11 17:50

Hi Dave,

As I explained you just undid a cumulative rounding fix that exposed the underlying bug in copybits. The underlying bug should be fixed.

The change to morphic extent: was probably one of mine.

I am uploading a changeset that does that CopyBitFix-wiz. It provides an asNonFraction method to Fraction and Number. Then uses it to adjust Origins and Extents inside the BitBlt ivars. That means that people can use fractions and it will be okay. BitBlt has the responsibility to check for parameters it can't handle.
 
(0014122 - 115 - 127 - 127 - 127 - 127 - 127)
wiz
05-23-11 18:14

Hmm. Apparently not all the way there yet.

The fix works for extents but not origins. Back to the drawing board.
 
(0014123 - 397 - 433 - 433 - 433 - 433 - 433)
wiz
05-23-11 19:32

Uploaded copyBitsFix-wiz.6.cs

This should do it. Tests included (and green). The last problem was solved by letting object (and therefore nil) understand asNonFraction.

The two main tests will fail before the fix and pass afterwards. The test for floating fractions should pass before and after. I needed it to debug the other test failure.

Yours in curiosity and service, --Jerome Peace
 
(0014124 - 230 - 254 - 254 - 254 - 254 - 254)
wiz
05-23-11 19:37

[OT] Discussing bugs on the list would be a lot easier if the list was a Mantis user and could be notified by mantis.

Some precautions would be needed to avoid to much traffic.

Yours in curiosity and service, --Jerome Peace.
 
(0014125 - 426 - 444 - 444 - 444 - 444 - 444)
wiz
05-23-11 22:55

Hmmm. looking around the other methods I see a lot of almost repetitions. I will look into doing some refactoring with the aim or rationalizing some of that.

It will be useful to harvest some of the changes. The asFraction methods need to be in place preliminary to any more serious changes. The tests are also useful.
copybits has accumulated a lot of unintegrated patches and thats what is worth focusing effort on next.
 
(0014126 - 403 - 469 - 469 - 469 - 469 - 469)
wiz
05-24-11 00:17
edited on: 05-24-11 00:18

Version 8

Includes lazy initialization in the adjust methods. Corrects the error message in copyBits.
Lets clipRange call copybits recursively without doing the horrible rounding That is saved as the last resort after the error message. Uses the adjust methods to refactor the lazy initialization out of clipRange.

The new tests still run green.

I think I am done for now.

Cheers, --Jer

 
(0014127 - 106 - 118 - 118 - 118 - 118 - 118)
wiz
05-24-11 00:49

Version 11 corresponds to fix now in inbox.

Differs from version 8 by minor fix to a clipRange comment.
 
(0014128 - 2478 - 2683 - 2683 - 2683 - 2683 - 2683)
wiz
05-25-11 17:10
edited on: 05-25-11 19:07

Bert and I are having an off list discussion about the fix. One of the valuable things that came out of it was a description of what copybits did and what I changed it to do. That is worth archiving here as well.


The use case I fixed says:

"copyBits should be able to handle any numerical argument with out raising an error. That is what the tests I wrote test for."

This is a big step forward. It means I can go back to removing inappropriate rounding at higher levels with confidence.

What you are asking me to address BEFORE you accept this fix is:

"copyBits should insure that all parameters are made into integers before being sent to primitive."

It has never done that in the past. It wasn't designed to do that. It isn't written that way. The use of copyBits is this:

"Try the primitive with the parameters as the user gave them to you.
If that doesn't work. Unhibernate one of the forms and try the primitive again. If that doesn't work unhibernate another one of the forms and try the primitive again. If that that doesn't work unhibernate the third form and try the primitive again. If that doesn't work see if the rule is oldPaint if so go to paintBits. Or if oldErase1bitShape go to eraseBits.
Else if we have a colormap change it to colorMap colors and try the primitive again.


If that doesn't work then clipRange if that changes anything then roundVariables and try the primitive one last time. Else send error message.
Proceed from error message at user request (or crash system till you fixed that.) Proceed by rounding the variables and trying the primitive one last time.

My changes changes that last paragraph to instead:

adjustExtents (assure heights and widths are defined and nonFraction) if adjustExtents changed anything try copyBits again. If that doesn't work adustOrigins (assure origin Xs and Ys are defined and nonFraction) if adjustOrigins changed anything try copyBits again. If that doesn't work
clipRange if clipRange changed anything try copyBits again. (this is what it used to do before initials yo wrote roundVariables) If that doesn't work give error message. Proceed from error at user request by roundingVariables and trying the primitive one last time.

I am convinced that roundVariables is written incorrectly. It is worth fixing but I have not addressed that yet BECAUSE testing is best done one step at a time. This step I have delayed its use until all other remedies have been exhausted.

 
(0014129 - 468 - 498 - 498 - 498 - 498 - 498)
wiz
05-25-11 17:19

copyBits is written as a recursive routine. It is a little strange because it recurses before rather than after trying all the tests. It works because each test modifies something s.t. the test will pass the next time through the recursion thus avoiding an infinite loop. The choice to do it this way allows copyBits to save time by not having to do the tests if the parameters are okay.


Okay now it is explained.

Yours in curiosity and service, --Jerome Peace
 
(0014133 - 69 - 69 - 69 - 69 - 69 - 69)
wiz
05-30-11 23:08

Added references to the bugs that required extent not to do rounding.
 
(0014139 - 256 - 286 - 646 - 646 - 646 - 646)
bert
06-06-11 11:03

Closing as Won't Fix. For discussion, see

http://lists.squeak.org/pipermail/squeak-dev/2011-May/160093.html [^]
http://lists.squeak.org/pipermail/squeak-dev/2011-May/160105.html [^]

Note: Kernel-wiz.592.mcz adding asNonFraction has been accepted into Trunk.
 

- Issue History
Date Modified Username Field Change
05-21-11 04:24 wiz New Issue
05-21-11 04:24 wiz Status new => assigned
05-21-11 04:24 wiz Assigned To  => andreas
05-21-11 04:24 wiz File Added: SqueakDebug200105202345.log
05-21-11 04:35 wiz Note Added: 0014113
05-21-11 22:39 lewis Issue Monitored: lewis
05-22-11 17:56 lewis Note Added: 0014114
05-22-11 20:15 lewis Note Added: 0014115
05-22-11 20:30 lewis Status assigned => resolved
05-22-11 20:30 lewis Resolution open => fixed
05-22-11 20:30 lewis Note Added: 0014116
05-22-11 21:48 wiz Note Added: 0014118
05-22-11 21:51 wiz Status resolved => feedback
05-22-11 21:51 wiz Resolution fixed => reopened
05-22-11 21:51 wiz Note Added: 0014119
05-22-11 22:20 lewis Note Added: 0014120
05-23-11 17:50 wiz Note Added: 0014121
05-23-11 17:52 wiz File Added: copyBitsFix-wiz.4.cs
05-23-11 18:14 wiz Note Added: 0014122
05-23-11 19:27 wiz File Added: copyBitsFix-wiz.6.cs
05-23-11 19:32 wiz Note Added: 0014123
05-23-11 19:37 wiz Note Added: 0014124
05-23-11 22:55 wiz Note Added: 0014125
05-24-11 00:12 wiz File Added: copyBitsFix-wiz.8.cs
05-24-11 00:17 wiz Note Added: 0014126
05-24-11 00:18 wiz Note Edited: 0014126
05-24-11 00:47 wiz File Added: copyBitsFix-wiz.11.cs
05-24-11 00:49 wiz Note Added: 0014127
05-25-11 17:10 wiz Note Added: 0014128
05-25-11 17:19 wiz Note Added: 0014129
05-25-11 19:07 wiz Note Edited: 0014128
05-30-11 23:07 wiz Relationship added related to 0006486
05-30-11 23:07 wiz Relationship added related to 0007213
05-30-11 23:08 wiz Note Added: 0014133
06-06-11 11:03 bert Status feedback => closed
06-06-11 11:03 bert Note Added: 0014139
06-06-11 11:03 bert Resolution reopened => won't fix
06-06-11 11:03 bert Fixed in Version  => 4.2


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