paintopsettings refactoring

LukasT.dev@gmail.com lukast.dev at gmail.com
Mon Jan 18 00:34:29 CET 2010


> Originally my plan was to remove the remove that access. For practical
> reasons there is still a dependency for the use in paintOutline and
> changePaintOpSize, as it's the best way to access the widgets from the
>  tool. It wouldn't be needed in paintOutline if it reads the setting, but
>  it turned that it too slow in case you change the paintop size on canvas
>  so it's still reading from the widget directly. For the properties itself
>  the widgets shouldn't be used.

I think that the settings object could be used.
I used to pass the KisPaintOpSettings object to paintop
and paintop asked for diameter like m_settings->shape() somewhere in a loop 
and it was ok, around 1200 instruction fetches, it was call to value() on some 
widget.

Now it uses the QStrings ID's
e.g. m_settings->getInt(SPRAYSHALE_SHAPE) and it eats 300 000 instruction 
fetches because of QSting ID. So we all know that it is not good to use it 
this way as you can see it is slow. 

SPRAYSHAPE_SHAPE is const QString variable. I stared to put that code in 
headers of the options as it was discussed. 

Here are some measures: http://lukast.mediablog.sk/callgrind/callgrind-
spray-01-18.out.tar.gz on spray.

So let's make the Settings object crate for basic data types.
Let's copy the setting object into primitive data types in this object 
and pass it to the paintop with copied data, so that programmer does not make 
a mistake and call the QString ID methods more then just once.

What do you think?

Disadvantage:
- more boiler-plate code 

Advantage:
+ speed
+ avoid coder mistakes 

> 
> > Problems:
> > * It is very frustrating job to port my paintops as I have many options
> > in GUI. It is easy to make mistake because of the code like
> > getInt("Spray/diaemter") compiles fine, but it has mistake (it suppose to
> > be
> > diameter). Is there any nice way to fprint the KisPaintOpSettings?
> 
> I know it's a bit frustrating with all those settings, but for the presets
> to work everythings must be serializable.
> The settings have a method dump() that outputs all properties. To avoid
> mistakes you can either define some strings and use these instead of
> hardcoding the strings or write a unittest that check the roundtrip of all
> properties.

Yes, then I discovered dump(), I changed it to const.
Does the KisPaintOpSettings still has to be const object? Why?
 
> > * I wanted to return QImage in the settings object because spray uses
> > QImage.But there isn't method getQImage("Spray/imageBrush") or something
> > like
> > this.
> 
> You can still put the QImage as a member in the sprayop settings.
> Everythings that isn't a property won't be saved automatically, so you have
> to do the saving code yourself. For a QImage the filename of the source
>  file could be added as property additionally to the QImage.
> 
> Another possibility would be to use a pattern from the pattern server
> instead of QImage. That way you only have to store the id of the pattern
>  and the paintop could fetch it from the server.
> 
> There is also the possibility that the image can't be loaded from disk when
> the preset is loaded, because it might be removed or something like that.
> For that I added a method canPaint() to KisPaintOp that can check if the
> settings are read correctly and block painting in case e.g. the image is
> missing.

KisSprayPaintOpSettings inherits from KisPropertiesConfiguration so I cast the 
setting object in writeOptionSetting and call setting->setImage(QImage) for 
now.

I would like to develop some nice widget for selecting bitmap brushes, 
something like the one for Gimp brushes, maybe the same, but I would like to 
add some other features like [x] "Compute the center of the brush".
Maybe add this for discussion with Peter :)
 
I would like to thank Sven for helping me on IRC with spray porting ;)


More information about the kimageshop mailing list