The Inbox: Graphics-kfr.434.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

The Inbox: Graphics-kfr.434.mcz

commits-2
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!


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Graphics-kfr.434.mcz

Levente Uzonyi
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!

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Graphics-kfr.434.mcz

Karl Ramberg
Thanks for the suggestion. I uploaded a new version based on it.

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!



Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Graphics-kfr.434.mcz

Karl Ramberg
In reply to this post by Levente Uzonyi
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.
Also looping with allButFirstDo: is slower than the original method.

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!



Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Graphics-kfr.434.mcz

Levente Uzonyi
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!
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: Graphics-kfr.434.mcz

Karl Ramberg
Yes, I tested with OrderedCollection.
Changed to Array and the speed was better.
It seems most senders of the Rectangle>>merging: are with Array eg: subBnds := Rectangle merging: (submorphs collect: [:m | m fullBounds]).
But DamageRecorder is an OrderedCollection for example.

I'm not sure how big impact speed will have here though.
I can't measure speed difference for small collections with up to 5000 elements (which will probably cover most use cases)

Change set in attachment.

Best,
Karl

On Sun, Jul 12, 2020 at 4:12 PM Levente Uzonyi <[hidden email]> wrote:
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!
>
>
>



RectangleMergeTesting.1.cs (3K) Download Attachment