2011/3/1 Nicolas Cellier <nicolas.cellier.aka.nice at gmail.com>:
> Nice catch.
>
> I think a simple solution would be to prevent mutation of colorRamp
> inst. var, and replace mutation with a copy.
> I don't see any use for these associations to be shared...
> There are currently two mutators:
> #firstColor:forMorph:hand: and #lastColor:forMorph:hand:
>
> Another solution would be to put a colorRamp deepCopy in the factory
> like you proposed
> initPixelRampCache.
>        ^PixelRampCache := LRUCache size: 32 factory:[:key|
>                (GradientFillStyle new colorRamp: key deepCopy) computePixelRampOfSize: 512]

Oops, this was stupid...
Of course, the copy protection here does not protect anything...
The protection should lie in pixelRamp

pixelRamp
	"Compute a pixel ramp, and cache it for future accesses"

	^pixelRamp ifNil:[
		"Insure the PixelRampCache is in place"
		PixelRampCache ifNil:[ self class initPixelRampCache  ].
		"Ask my cache for an existing instance if one is available"
		pixelRamp := PixelRampCache at: colorRamp deepCopy	
	].

Nicolas

>
> I don't think that protecting the LRUCache is a good idea.
> #deepCopy may cause infinite loop on object cyclic graphs
> IMO the programmer shall be in charge of copying.
>
> Anyway, the hash is inefficient indeed, most colorRamps share the same keys...
> {
> (((GradientFillStyle classPool at: #PixelRampCache) instVarAt:
> (LRUCache allInstVarNames indexOf: 'values')) collect: [:e | e at: 2]
> as: Set) size.
> ((GradientFillStyle classPool at: #PixelRampCache) instVarAt:
> (LRUCache allInstVarNames indexOf: 'values')) size.
> }
> -> #(7 32)
>
> I have no immediate solution for this one...
>
> Nicolas
>
> 2011/3/1 glenpaling <glenpaling at rogers.com>:
>> Modifications to Morphic gradients fail in current versions of Squeak (and
>> Pharo). Using the halo menu to change the first or last color of a morph
>> doesn't work. Despite changes to these colors, the initial gradient remains.
>>
>> I've traced the problem to false hits in GradientFillStyle's LRUCache.
>> LRUCache acts similar to a dictionary. It uses a key and it's hash to store
>> and retrieve values. GradientFillStyle uses uses its colorRamp--a mutable
>> array of associations--as the key to the cache, for instance: {0.0->Color
>> white. 1.0->Color red}. Subsequent modifications to colorRamp also changes
>> the key IN the cache. This can result in false hits.
>>
>> This fault has always been present. Recent changes to the hash calculation
>> in Associations increased its frequency. Association hash has been
>> simplified to return the key. As a result all gradients created from the
>> user interface have the same hash. Since the the lookup == the key and the
>> hashes are the same, false hits are inevitable.
>>
>> To illustrate the problem execute the script below. The first two changes
>> are done using the classes colorRamp modification function. Rather than
>> modifying, the last change replaces colorRamp with a new one. Clearly the
>> first item in the pixelRamp should change when the first color changes.
>>
>> gradient := GradientFillStyle new colorRamp: {0.0->Color red. 1.0->Color
>> white}.
>> Transcript
>>        << 'first item for: ';
>>        << gradient colorRamp printString; space;
>>        << gradient pixelRamp first;
>>        cr; flush.
>>
>> gradient firstColor: Color green forMorph: nil hand: nil.
>> Transcript
>>        << 'first item for: ';
>>        << gradient colorRamp printString; space;
>>        << gradient pixelRamp first;
>>        cr; flush.
>>
>> gradient firstColor: Color yellow forMorph: nil hand: nil.
>> Transcript
>>        << 'first item for: ';
>>        << gradient colorRamp printString; space;
>>        << gradient pixelRamp first;
>>        cr; flush.
>>
>> Transcript show: 'Now replace with a new colorRamp'; cr.
>> newRamp := gradient colorRamp deepCopy.
>> newRamp first value: Color black.
>> gradient colorRamp: newRamp.
>> Transcript
>>        << 'first item for: ';
>>        << gradient colorRamp printString; space;
>>        << gradient pixelRamp first;
>>        cr; flush.
>>
>>
>> Results:
>>
>> first item for: {0.0->Color red . 1.0->Color white} 4294901760
>> first item for: {0.0->Color green . 1.0->Color white} 4294901760
>> first item for: {0.0->Color yellow . 1.0->Color white} 4294901760
>> Now replace with a new colorRamp
>> first item for: {0.0->Color black . 1.0->Color white} 4278190081
>>
>>
>> What's the best way to fix this? Handing out your internals seems ill
>> advised. I think GradientFillStyle should pass a deepCopy of colorRamp to
>> LRUCache. Another solution is for LRUCache to make a deepCopy itself. This
>> would provide protection for all users of LRUCache...provided that deepCopy
>> will always be sufficient.
>>
>>
>> Glen
>>
>> --
>> View this message in context: http://forum.world.st/Gradient-Fills-Failures-tp3330016p3330016.html
>> Sent from the Squeak - Dev mailing list archive at Nabble.com.
>>
>>
>