paintopsettings refactoring

Sven Langkamp sven.langkamp at gmail.com
Mon Jan 18 09:12:02 CET 2010


On Mon, Jan 18, 2010 at 12:34 AM, LukasT.dev at gmail.com <lukast.dev at gmail.com
> wrote:

> > 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<http://lukast.mediablog.sk/callgrind/callgrind-%0Aspray-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
>

Very interesting measurements. From the callgrind file I can see two
problems: Spray brush reads settngs while painting and curve options are way
to verbose.

The settings should be read once the paintop is created and store in the
paintop. Doing that in the settings has the disadvantaage that you have to
keep it in sync with the properties that are written. In the paintop you can
just read the settings from the setting object and then forget about the
object.
Callgrind shows 5 properties that are read 465 times while painting, all
other properties are already stored in the spray brush.

Reading the curve option properties take way too long. At the moment every
curve option stores 256 doubles as single properties and there are three in
spray alone. So spray currently has over 800 properties in the settings.
Curve options need to store the control points instead of the final results
(also to write the back to the UI from the setting) and store the container
directly instead of putting the single values into a QVariant.

Beside that these things look very small compared to the impact of
KisPainter::fillPainterPath creating 465 temporary paintdevices while
painting. Some optimization potential there, but that's another topic.

>
> > > 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?


Does it need to be non-const? Not for dump at least.

> > * 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 :)
>

We can have a discussion at the sprint about that. I'm sure there will be
some input from the usability discussion too.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20100118/6dbea65a/attachment.htm 


More information about the kimageshop mailing list