The Inbox: Graphics-kfr.434.mcz

Hi Karl,

While both changes look right, they modify the behavior of those methods
when their argument is an empty list.
Instead of raising an error, they'll return a Rectangle with both fields
set to nil.
If the argument is really a list - aka SequenceableCollection, I suggest
using more specific methods instead of #do: to process the list in order
to trigger the error as soon as possible.
For example, use #first to get the first element and #allButFirstDo: to
iterate over the rest. That would simplify the method as well by making
the nil checks unnecessary.


Levente

On Sat, 11 Jul 2020, [hidden email] wrote:

> A new version of Graphics was added to project The Inbox:
> http://source.squeak.org/inbox/Graphics-kfr.434.mcz
>
> ==================== Summary ====================
>
> Name: Graphics-kfr.434
> Author: kfr
> Time: 11 July 2020, 10:18:06.272175 am
> UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84
> Ancestors: Graphics-mt.433
>
> I wanted to subclass Rectangle with a instance variable to carry some state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new subclass SuperRectangle
>
> =============== Diff against Graphics-mt.433 ===============
>
> Item was changed:
>  ----- Method: Rectangle class>>encompassing: (in category 'instance creation') -----
>  encompassing: listOfPoints
>       "A number of callers of encompass: should use this method."
>       | topLeft bottomRight |
>       topLeft := bottomRight := nil.
>       listOfPoints do:
>               [:p | topLeft == nil
>                       ifTrue: [topLeft := bottomRight := p]
>                       ifFalse: [topLeft := topLeft min: p.
>                                       bottomRight := bottomRight max: p]].
> +     ^self origin: topLeft corner: bottomRight!
> -     ^ topLeft corner: bottomRight!
>
> Item was changed:
>  ----- Method: Rectangle class>>merging: (in category 'instance creation') -----
>  merging: listOfRects
>       "A number of callers of merge: should use this method."
>       | minX minY maxX maxY |
>       listOfRects
>               do: [:r | minX
>                               ifNil: [minX := r topLeft x. minY := r topLeft y.
>                                       maxX := r bottomRight x. maxY := r bottomRight y]
>                               ifNotNil: [minX := minX min: r topLeft x. minY := minY min: r topLeft y.
>                                       maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]].
> +     ^ self origin:minX@minY corner: maxX@maxY!
> -     ^ minX@minY corner: maxX@maxY!

Hi Karl,

While both changes look right, they modify the behavior of those methods
when their argument is an empty list.
Instead of raising an error, they'll return a Rectangle with both fields
set to nil.
If the argument is really a list - aka SequenceableCollection, I suggest
using more specific methods instead of #do: to process the list in order
to trigger the error as soon as possible.
For example, use #first to get the first element and #allButFirstDo: to
iterate over the rest. That would simplify the method as well by making
the nil checks unnecessary.


Levente

On Sat, 11 Jul 2020, [hidden email] wrote:

> A new version of Graphics was added to project The Inbox:
> http://source.squeak.org/inbox/Graphics-kfr.434.mcz
>
> ==================== Summary ====================
>
> Name: Graphics-kfr.434
> Author: kfr
> Time: 11 July 2020, 10:18:06.272175 am
> UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84
> Ancestors: Graphics-mt.433
>
> I wanted to subclass Rectangle with a instance variable to carry some state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new subclass SuperRectangle
>
> =============== Diff against Graphics-mt.433 ===============
>
> Item was changed:
>  ----- Method: Rectangle class>>encompassing: (in category 'instance creation') -----
>  encompassing: listOfPoints
>       "A number of callers of encompass: should use this method."
>       | topLeft bottomRight |
>       topLeft := bottomRight := nil.
>       listOfPoints do:
>               [:p | topLeft == nil
>                       ifTrue: [topLeft := bottomRight := p]
>                       ifFalse: [topLeft := topLeft min: p.
>                                       bottomRight := bottomRight max: p]].
> +     ^self origin: topLeft corner: bottomRight!
> -     ^ topLeft corner: bottomRight!
>
> Item was changed:
>  ----- Method: Rectangle class>>merging: (in category 'instance creation') -----
>  merging: listOfRects
>       "A number of callers of merge: should use this method."
>       | minX minY maxX maxY |
>       listOfRects
>               do: [:r | minX
>                               ifNil: [minX := r topLeft x. minY := r topLeft y.
>                                       maxX := r bottomRight x. maxY := r bottomRight y]
>                               ifNotNil: [minX := minX min: r topLeft x. minY := minY min: r topLeft y.
>                                       maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]].
> +     ^ self origin:minX@minY corner: maxX@maxY!
> -     ^ minX@minY corner: maxX@maxY!

Hi Karl,

On Sun, 12 Jul 2020, karl ramberg wrote:

> Hi,
>
> In Rectangle>>merging: it is quite a bit slower to use points topLeft and bottomRight, than reading out the min and max of each side.

Indeed. Each iteration creates a new point if you use points.

> Also looping with allButFirstDo: is slower than the original method.

I suppose your list of rectangles was an OrderedCollection when you
measured performance because for most SequenceableCollections
#allButFirstDo: is slightly faster than #do: by definition.
OrderedCollection overrides #do: for better performance but does not
have such implementation for #allButFirstDo:/#allButLastDo:.


Levente

>
> This seems to be fastest:
>
> Rectangle>>merging: listOfRects
> "A number of callers of merge: should use this method."
> | minX minY maxX maxY |
> minX := listOfRects first topLeft x.
> minY := listOfRects first topLeft y.
> maxX := listOfRects first bottomRight x.
> maxY := listOfRects first bottomRight y.
> listOfRects do: [:r | minX := minX min: r topLeft x.
>              minY := minY min: r topLeft y.
>              maxX := maxX max: r bottomRight x.
>              maxY := maxY max: r bottomRight y].
> ^self origin: minX@minY corner: maxX@maxY
>
> Best,
> Karl
>
> On Sat, Jul 11, 2020 at 3:49 PM Levente Uzonyi <[hidden email]> wrote:
>       Hi Karl,
>
>       While both changes look right, they modify the behavior of those methods
>       when their argument is an empty list.
>       Instead of raising an error, they'll return a Rectangle with both fields
>       set to nil.
>       If the argument is really a list - aka SequenceableCollection, I suggest
>       using more specific methods instead of #do: to process the list in order
>       to trigger the error as soon as possible.
>       For example, use #first to get the first element and #allButFirstDo: to
>       iterate over the rest. That would simplify the method as well by making
>       the nil checks unnecessary.
>
>
>       Levente
>
>       On Sat, 11 Jul 2020, [hidden email] wrote:
>
>       > A new version of Graphics was added to project The Inbox:
>       > http://source.squeak.org/inbox/Graphics-kfr.434.mcz
>       >
>       > ==================== Summary ====================
>       >
>       > Name: Graphics-kfr.434
>       > Author: kfr
>       > Time: 11 July 2020, 10:18:06.272175 am
>       > UUID: 0e0125f6-3040-6247-9e5a-43bfd1896f84
>       > Ancestors: Graphics-mt.433
>       >
>       > I wanted to subclass Rectangle with a instance variable to carry some state, but these methodes indirected to Point>>corner: so it broke the override and returned a ordinary Rectangle instead of my fancy new
>       subclass SuperRectangle
>       >
>       > =============== Diff against Graphics-mt.433 ===============
>       >
>       > Item was changed:
>       >  ----- Method: Rectangle class>>encompassing: (in category 'instance creation') -----
>       >  encompassing: listOfPoints
>       >       "A number of callers of encompass: should use this method."
>       >       | topLeft bottomRight |
>       >       topLeft := bottomRight := nil.
>       >       listOfPoints do:
>       >               [:p | topLeft == nil
>       >                       ifTrue: [topLeft := bottomRight := p]
>       >                       ifFalse: [topLeft := topLeft min: p.
>       >                                       bottomRight := bottomRight max: p]].
>       > +     ^self origin: topLeft corner: bottomRight!
>       > -     ^ topLeft corner: bottomRight!
>       >
>       > Item was changed:
>       >  ----- Method: Rectangle class>>merging: (in category 'instance creation') -----
>       >  merging: listOfRects
>       >       "A number of callers of merge: should use this method."
>       >       | minX minY maxX maxY |
>       >       listOfRects
>       >               do: [:r | minX
>       >                               ifNil: [minX := r topLeft x. minY := r topLeft y.
>       >                                       maxX := r bottomRight x. maxY := r bottomRight y]
>       >                               ifNotNil: [minX := minX min: r topLeft x. minY := minY min: r topLeft y.
>       >                                       maxX := maxX max: r bottomRight x. maxY := maxY max: r bottomRight y]].
>       > +     ^ self origin:minX@minY corner: maxX@maxY!
>       > -     ^ minX@minY corner: maxX@maxY!
>
>
>