The Trunk: MorphicExtras-nice.137.mcz
Locked
 
|
Nicolas Cellier uploaded a new version of MorphicExtras to project The Trunk:
http://source.squeak.org/trunk/MorphicExtras-nice.137.mcz ==================== Summary ==================== Name: MorphicExtras-nice.137 Author: nice Time: 13 December 2013, 10:07:57.316 pm UUID: 127ce68e-b210-42a3-b0b3-b40c69941d02 Ancestors: MorphicExtras-nice.136 Correct an awfull bug in PaintBoxMorph>>showColor One should NEVER send pixelValueAt: nor colorAt: to an hibernated Form (or ColorForm). =============== Diff against MorphicExtras-nice.136 =============== Item was changed: ----- Method: PaintBoxMorph>>showColor (in category 'actions') ----- showColor "Display the current color in all brushes, both on and off." | offIndex onIndex center | currentColor ifNil: [^self]. "colorPatch color: currentColor. May delete later" (brushes isNil or: [brushes first owner ~~ self]) ifTrue: [brushes := OrderedCollection new. #(#brush1: #brush2: #brush3: #brush4: #brush5: #brush6:) do: [:sel | brushes addLast: (self submorphNamed: sel)]]. + brushes last offImage unhibernate. + brushes last onImage unhibernate. + brushes last pressedImage unhibernate. + center := brushes last offImage extent // 2. + offIndex := brushes last offImage pixelValueAt: center. + onIndex := brushes last onImage pixelValueAt: center. - center := (brushes sixth) offImage extent // 2. - offIndex := (brushes sixth) offImage pixelValueAt: center. - onIndex := (brushes sixth) onImage pixelValueAt: center. brushes do: [:bb | bb offImage colors at: offIndex + 1 put: currentColor. bb offImage clearColormapCache. bb onImage colors at: onIndex + 1 put: currentColor. bb onImage clearColormapCache. bb invalidRect: bb bounds]. self invalidRect: (brushes first topLeft rect: brushes last bottomRight)! |
Re: The Trunk: MorphicExtras-nice.137.mcz
|
|
I feel like it would be better to change pixelValueAt: so as to always unhibernate, but I did not dare if ever someone had the idea of sending it in tight loops (one shouldn't, there is BitBlt for that purpose, but who knows...).
Maybe we could fail primitivePixelValueAt if receiver is Byte like, and unhibernate in fallback code. We would let every other BitBlt primitives accept a ByteArray to allow BitBlt tricks. Thoughts? 2013/12/13 <[hidden email]> Nicolas Cellier uploaded a new version of MorphicExtras to project The Trunk: |
Re: The Trunk: MorphicExtras-nice.137.mcz
|
|
2013/12/13 Nicolas Cellier <[hidden email]>
No one raised a comment so far. Isn't it a good idea to fail the primitive? If yes, then it's very simple to implement, just add: (interpreterProxy isWords: bitmap) ifFalse:[interpreterProxy primitiveFail]. somewhere in the preamble of BitBltSimulation>>primitivePixelValueAtX: xVal y: yVal I check the senders of pixelValueAt:, and some of them would loop over each pixel. So sending unhibernate at each pixel does not seem a good idea anyway. Leaking unhibernate sends out of the loop neither is, because then 3rd party classes must be aware of an implementation detail of Form.
|
Re: The Trunk: MorphicExtras-nice.137.mcz
|
|
On 19-12-2013, at 3:55 PM, Nicolas Cellier <[hidden email]> wrote: > Isn't it a good idea to fail the primitive? > If yes, then it's very simple to implement, just add: > > (interpreterProxy isWords: bitmap) ifFalse:[interpreterProxy primitiveFail]. > > somewhere in the preamble of BitBltSimulation>>primitivePixelValueAtX: xVal y: yVal sounds plausible to me; probably ought to have been there at the beginning. Fail the prim, have backup code unhibernate and retry. Better yet, fail with an error code and have the backup code test for it. tim -- tim Rowledge; [hidden email]; http://www.rowledge.org/tim Don't sweat petty things....or pet sweaty things. |
Re: The Trunk: MorphicExtras-nice.137.mcz
|
|
In reply to this post by Nicolas Cellier
On 20.12.2013, at 00:55, Nicolas Cellier <[hidden email]> wrote:
The primitive must fail, yes. That is simply a bug. Before that primitive existed, pixelValueAt: used BitBlt which did the unhibernate.
IMHO the primitive should do the same check as BitBlt: verify that the size of the bits is exactly right given width, height, and depth. - Bert -
|
Re: The Trunk: MorphicExtras-nice.137.mcz
|
|
2013/12/20 Bert Freudenberg <[hidden email]>
|
Re: The Trunk: MorphicExtras-nice.137.mcz
|
|
On Fri, Dec 20, 2013 at 02:13:29AM +0100, Nicolas Cellier wrote:
> 2013/12/20 Bert Freudenberg <[hidden email]> > > > > > On 20.12.2013, at 00:55, Nicolas Cellier < > > [hidden email]> wrote: > > > > 2013/12/13 Nicolas Cellier <[hidden email]> > > > >> I feel like it would be better to change pixelValueAt: so as to always > >> unhibernate, but I did not dare if ever someone had the idea of sending it > >> in tight loops (one shouldn't, there is BitBlt for that purpose, but who > >> knows...). > >> > >> Maybe we could fail primitivePixelValueAt if receiver is Byte like, and > >> unhibernate in fallback code. > >> We would let every other BitBlt primitives accept a ByteArray to allow > >> BitBlt tricks. > >> > >> Thoughts? > >> > >> > > No one raised a comment so far. > > Isn't it a good idea to fail the primitive? > > > > > > The primitive must fail, yes. That is simply a bug. Before that primitive > > existed, pixelValueAt: used BitBlt which did the unhibernate. > > > > If yes, then it's very simple to implement, just add: > > > > (interpreterProxy isWords: bitmap) ifFalse:[interpreterProxy > > primitiveFail]. > > > > > > IMHO the primitive should do the same check as BitBlt: verify that the > > size of the bits is exactly right given width, height, and depth. > > > > - Bert - > > > > I opened http://bugs.squeak.org/view.php?id=7799 just in case. > Thanks for opening the bugs.squeak.org issue. Your patch adds this: bitsSize := interpreterProxy byteSizeOf: bitmap. ((interpreterProxy isWordsOrBytes: bitmap) and: [bitsSize = (stride * height * 4 "bytes per word")]) ifFalse: [^interpreterProxy primitiveFail]. Which translates as: bitsSize = interpreterProxy->byteSizeOf(bitmap); if (!((interpreterProxy->isWordsOrBytes(bitmap)) && (bitsSize == ((stride * height) * 4)))) { interpreterProxy->primitiveFail(); return null; } This looks like exactly what Bert was suggesting. I think there are two things we should test: 1) Check if the extra computation has an effect on performance. 2) Check to make sure the logic works on the 64 bit image, to make sure that the 4 bytes per word does the right thing (I think that it will work, and I can follow up on this later to be sure). Dave |
Re: The Trunk: MorphicExtras-nice.137.mcz
|
|
On Fri, Dec 20, 2013 at 10:14:11AM -0500, David T. Lewis wrote:
> On Fri, Dec 20, 2013 at 02:13:29AM +0100, Nicolas Cellier wrote: > > > > I opened http://bugs.squeak.org/view.php?id=7799 just in case. > > > > I think there are two things we should test: > > 1) Check if the extra computation has an effect on performance. > > 2) Check to make sure the logic works on the 64 bit image, to make sure > that the 4 bytes per word does the right thing (I think that it will work, > and I can follow up on this later to be sure). > Just to follow up on #2, I wrote a test primitive that confirms that the logic works correctly on a 64-bit object memory. So no worries there, and as previously discussed, performance is not a problem. Dave |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |