The Inbox: Graphics-wiz.183.mcz

> A new version of Graphics was added to project The Inbox:
> http://source.squeak.org/inbox/Graphics-wiz.183.mcz
>
> ==================== Summary ====================
>
> Name: Graphics-wiz.183
> Author: wiz
> Time: 23 May 2011, 8:55:06.627 pm
> UUID: de71490d-a20e-4413-b8b0-8bc9ae3bfdb9
> Ancestors: Graphics-nice.182
>
> The last of three pieces fixing M7635.
>
> adjustOrigins and adjustExtent assure parameters are defined and not in class Fraction form.
> clipRange has had lazy initialization factored out by useing the adjust methods.
> copyBits now tries the two adjust methods before trying cliprange. It also now tries clipRange without the horrible roundVariables method. As a last resort it sends and error method before proceeding to try the roundVariables method.
>
> That should do it.
>
> Oh yeah be sure to load the other two parts first. Everything will depend on the presense of asNonFraction.
>
> =============== Diff against Graphics-nice.182 ===============
>
> Item was added:
> + ----- Method: BitBlt>>adjustExtents (in category 'accessing') -----
> + adjustExtents
> + "Answer if any of the extent numbers were class fraction or undefined.
> + Assure all  extent numbers are  defined and non-fractional."
> +
> + | answer |
> + (answer := width isFraction | height isFraction | clipWidth isFraction | clipHeight isFraction
> + | width isNil | height isNil | clipWidth isNil | clipHeight isNil) ifFalse: [ ^answer ] .
> + width := ( width ifNil: [ destForm width ] ) asNonFraction .
> + height := ( height ifNil: [ destForm height ] ) asNonFraction .
> + clipWidth := (clipWidth ifNil:[clipWidth := destForm width]) asNonFraction .
> + clipHeight := (clipHeight ifNil:[clipHeight := destForm height]) asNonFraction .
> + ^answer !
>
> Item was added:
> + ----- Method: BitBlt>>adjustOrigins (in category 'accessing') -----
> + adjustOrigins
> + "Answer if any of the origin numbers were class Fraction or undefined. Assure all  origin numbers are non-fractional and defined."
> +
> + | answer |
> + (answer := destX isFraction | destY isFraction
> + | sourceX isFraction | sourceY isFraction
> + | clipX isFraction | clipY isFraction
> + | destX isNil | destY isNil
> + | sourceX isNil | sourceY isNil
> + | clipX isNil | clipY isNil )
> + ifFalse: [ ^answer ] .
> +
> + destX :=( destX ifNil:[ 0]) asNonFraction .
> + destY := (destY  ifNil:[ 0]) asNonFraction .
> + sourceX := (sourceX  ifNil:[ 0]) asNonFraction .
> + sourceY := (sourceY  ifNil:[ 0]) asNonFraction .
> + clipX := (clipX  ifNil:[0]) asNonFraction .
> + clipY := (clipY  ifNil:[0]) asNonFraction .
> + ^ answer!
>
> Item was changed:
>  ----- Method: BitBlt>>clipRange (in category 'private') -----
>  clipRange
> + "Fill in the lazy state if needed and insure no numbers are class Fraction.
> + Clip and adjust source origin and extent appropriately"
> +
> - "clip and adjust source origin and extent appropriately"
> - "first in x"
>   | sx sy dx dy bbW bbH |
> + self adjustExtents | self adjustOrigins . "True when something changed."
> - "fill in the lazy state if needed"
> - destX ifNil:[destX := 0].
> - destY ifNil:[destY := 0].
> - width ifNil:[width := destForm width].
> - height ifNil:[height := destForm height].
> - sourceX ifNil:[sourceX := 0].
> - sourceY ifNil:[sourceY := 0].
> - clipX ifNil:[clipX := 0].
> - clipY ifNil:[clipY := 0].
> - clipWidth ifNil:[clipWidth := destForm width].
> - clipHeight ifNil:[clipHeight := destForm height].
>
> + "first in x"
>   destX >= clipX
>   ifTrue: [sx := sourceX.
>   dx := destX.
>   bbW := width]
>   ifFalse: [sx := sourceX + (clipX - destX).
>   bbW := width - (clipX - destX).
>   dx := clipX].
>   (dx + bbW) > (clipX + clipWidth)
>   ifTrue: [bbW := bbW - ((dx + bbW) - (clipX + clipWidth))].
>   "then in y"
>   destY >= clipY
>   ifTrue: [sy := sourceY.
>   dy := destY.
>   bbH := height]
>   ifFalse: [sy := sourceY + clipY - destY.
>   bbH := height - (clipY - destY).
>   dy := clipY].
>   (dy + bbH) > (clipY + clipHeight)
>   ifTrue: [bbH := bbH - ((dy + bbH) - (clipY + clipHeight))].
>   sourceForm ifNotNil:[
>   sx < 0
>   ifTrue: [dx := dx - sx.
>   bbW := bbW + sx.
>   sx := 0].
>   sx + bbW > sourceForm width
>   ifTrue: [bbW := bbW - (sx + bbW - sourceForm width)].
>   sy < 0
>   ifTrue: [dy := dy - sy.
>   bbH := bbH + sy.
>   sy := 0].
>   sy + bbH > sourceForm height
>   ifTrue: [bbH := bbH - (sy + bbH - sourceForm height)].
>   ].
>   (bbW <= 0 or:[bbH <= 0]) ifTrue:[
>   sourceX := sourceY := destX := destY := clipX := clipY := width := height := 0.
>   ^true].
>   (sx = sourceX
>   and:[sy = sourceY
>   and:[dx = destX
>   and:[dy = destY
>   and:[bbW = width
>   and:[bbH = height]]]]]) ifTrue:[^false].
>   sourceX := sx.
>   sourceY := sy.
>   destX := dx.
>   destY := dy.
>   width := bbW.
>   height := bbH.
>   ^true!
>
> Item was changed:
>  ----- Method: BitBlt>>copyBits (in category 'copying') -----
>  copyBits
>   "Primitive. Perform the movement of bits from the source form to the
>   destination form. Fail if any variables are not of the right type (Integer,
>   Float, or Form) or if the combination rule is not implemented.
>   In addition to the original 16 combination rules, this BitBlt supports
>   16 fail (to simulate paint)
>   17 fail (to simulate mask)
>   18 sourceWord + destinationWord
>   19 sourceWord - destinationWord
>   20 rgbAdd: sourceWord with: destinationWord
>   21 rgbSub: sourceWord with: destinationWord
>   22 rgbDiff: sourceWord with: destinationWord
>   23 tallyIntoMap: destinationWord
>   24 alphaBlend: sourceWord with: destinationWord
>   25 pixPaint: sourceWord with: destinationWord
>   26 pixMask: sourceWord with: destinationWord
>   27 rgbMax: sourceWord with: destinationWord
>   28 rgbMin: sourceWord with: destinationWord
>   29 rgbMin: sourceWord bitInvert32 with: destinationWord
>  "
>   <primitive: 'primitiveCopyBits' module: 'BitBltPlugin'>
>
>   "Check for compressed source, destination or halftone forms"
>   (combinationRule >= 30 and: [combinationRule <= 31]) ifTrue:
>   ["No alpha specified -- re-run with alpha = 1.0"
>   ^ self copyBitsTranslucent: 255].
>   ((sourceForm isForm) and: [sourceForm unhibernate])
>   ifTrue: [^ self copyBits].
>   ((destForm isForm) and: [destForm unhibernate])
>   ifTrue: [^ self copyBits].
>   ((halftoneForm isForm) and: [halftoneForm unhibernate])
>   ifTrue: [^ self copyBits].
>
>   "Check for unimplmented rules"
>   combinationRule = Form oldPaint ifTrue: [^ self paintBits].
>   combinationRule = Form oldErase1bitShape ifTrue: [^ self eraseBits].
>
>   "Check if BitBlt doesn't support full color maps"
>   (colorMap notNil and:[colorMap isColormap]) ifTrue:[
>   colorMap := colorMap colors.
>   ^self copyBits].
> + "Check for class Fraction numbers and undefined paramenters."
> + self adjustExtents ifTrue: [ ^self copyBits ].
> + self adjustOrigins ifTrue: [ ^self copyBits ].
> +
>   "Check if clipping gots us way out of range"
> + self clipRange ifTrue:[ ^self copyBits].
> - self clipRange ifTrue:[self roundVariables. ^self copyBitsAgain].
>
> + self error: 'Bad BitBlt arg; proceed to round args..'.
> - self error: 'Bad BitBlt arg (Fraction?); proceed to convert.'.
>   "Convert all numeric parameters to integers and try again."
>   self roundVariables.
>   ^ self copyBitsAgain!
>
>

> From: Bert Freudenberg <[hidden email]>
> Subject: Re: [squeak-dev] The Inbox: Graphics-wiz.183.mcz
> To: "Jerome Peace" <[hidden email]>, "The general-purpose Squeak developers list" <[hidden email]>
> Date: Tuesday, May 24, 2011, 5:47 AM
> Hi Jerome,
>
> since BitBlt only works on integer coordinates anyway, and
> even accepts nil instead ints, why should you want to spend
> all that effort of converting Fractions to Floats?
>

>
>
> --- On Tue, 5/24/11, Bert Freudenberg <[hidden email]> wrote:
>
>> From: Bert Freudenberg <[hidden email]>
>> Subject: Re: [squeak-dev] The Inbox: Graphics-wiz.183.mcz
>> To: "Jerome Peace" <[hidden email]>, "The general-purpose Squeak developers list" <[hidden email]>
>> Date: Tuesday, May 24, 2011, 5:47 AM
>> Hi Jerome,
>>
>> since BitBlt only works on integer coordinates anyway, and
>> even accepts nil instead ints, why should you want to spend
>> all that effort of converting Fractions to Floats?
>>
> Hi Bert,
>
> As is the case with most of my debugging I have to do it in a context of learning as I go. Some of my design decisions are aimed at maximizing that learning.
>
> The goal here was to fix one bug in the right place. The primitive will fail if it receives a class Fraction but not if it receives the same number as a float. I don't wish to touch the primitive. I am not about to learn enough today to know how. I can make copyBits take responsibility for insuring the primitive is happy with the parameters it receives. The minimum I need to do is translate fractions when I get them into floats before they get to the primitive. The primitive handles floats on its own. I don't have to second guess or take responsibility away from it.
>
> As a rule it is always better not to increase entropy. I.E don't erase information. Rounding is doing that. It is also doing it in the dark. I don't know what the user intended. Also I don't want to filter anything I don't have to. Else I would never be able to test how the primitive handles fractions. For example, one curiosity is which pixels will it paint if I tell it x is 0.75 and the width is 4.5 ? I don't know the answer and I want to leave myself a way of finding out. Part of what I did was to leave in place ways of testing.
>
> If somebody creates a fractional send I need only guarantee it works. If I do that then if one of the 130 users of copyBits pulls information out of the Grafport or BitBlt I have not changed the essence of that information. I don't know and I don't have time to check how people are using or misusing things in squeak. The same logic goes for not forcing people to round to integers at a higher level. Once this fix is in place we can begin to tackle all the places that do that rounding inappropriately.
>
> This example you give may or may not be doing things correctly. To avoid the class fraction bug it may have skewed its design in favor of rounding/truncating to an integer. I am not ready to test or judge that yet. I want a way to do that when the time comes. If you look at roundVariables you see why I am cautious. I expect to find errors in others peoples code. I tested what happened when the error message was commented out androundVariables was invoked. I am still trying to explain why I saw what I saw.
>
> Cunningham's do the simplest thing that might work applies here. Once we gain experience with this fix we will know if we have to try something else. Fixes are incremental.
>
> I had enough time yesterday to produce this fix. As I learn more I might go farther.
>
> The asNonFraction stuff is not really more work but less. I learned to do that after writing asNonFractionalPoint. At some point I will be able to rewrite my first attempt using the new method in place of clumsy type checking. Using it means I don't have to change anything unless really necessary. Deferring any rounding decisions to the primitive. copyBits only needs to adjust extents if one is undefined or unacceptable. Ditto origins and clipRange. If I profile when changes are needed then I can get a picture of how the users of copyBits are using it and more importantly misusing it.  
>
> So I made design decisions with their usefulness to future work in mind.
>
> Part of the reason for stopping here is the fix was needed quickly to head of a worse fix that I am trying to convince Dave to revert. Part of being careful is years of experience tracking down the cause of gribbles and off by one pixel errors.
>
> Unfortunately squeak has a lot of sloppy maintainers. I have a right to say this. I have seen and cleaned up a good deal of graphic sloppiness. I have also seen a lot of graphic sloppiness that is not yet possible to clean up. This fix, exactly the way it is, will help towards the next step in that clean up process.
>
> I hope you will now support this fix and my general effort to reduce graphic bugs.