<div class="gmail_quote">On Mon, Jan 18, 2010 at 12:34 AM, <a href="mailto:LukasT.dev@gmail.com">LukasT.dev@gmail.com</a> <span dir="ltr"><<a href="mailto:lukast.dev@gmail.com">lukast.dev@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="im">> Originally my plan was to remove the remove that access. For practical<br>
> reasons there is still a dependency for the use in paintOutline and<br>
> changePaintOpSize, as it's the best way to access the widgets from the<br>
> tool. It wouldn't be needed in paintOutline if it reads the setting, but<br>
> it turned that it too slow in case you change the paintop size on canvas<br>
> so it's still reading from the widget directly. For the properties itself<br>
> the widgets shouldn't be used.<br>
<br>
</div>I think that the settings object could be used.<br>
I used to pass the KisPaintOpSettings object to paintop<br>
and paintop asked for diameter like m_settings->shape() somewhere in a loop<br>
and it was ok, around 1200 instruction fetches, it was call to value() on some<br>
widget.<br>
<br>
Now it uses the QStrings ID's<br>
e.g. m_settings->getInt(SPRAYSHALE_SHAPE) and it eats 300 000 instruction<br>
fetches because of QSting ID. So we all know that it is not good to use it<br>
this way as you can see it is slow.<br>
<br>
SPRAYSHAPE_SHAPE is const QString variable. I stared to put that code in<br>
headers of the options as it was discussed.<br>
<br>
Here are some measures: <a href="http://lukast.mediablog.sk/callgrind/callgrind-%0Aspray-01-18.out.tar.gz" target="_blank">http://lukast.mediablog.sk/callgrind/callgrind-<br>
spray-01-18.out.tar.gz</a> on spray.<br>
<br>
So let's make the Settings object crate for basic data types.<br>
Let's copy the setting object into primitive data types in this object<br>
and pass it to the paintop with copied data, so that programmer does not make<br>
a mistake and call the QString ID methods more then just once.<br>
<br>
What do you think?<br>
<br>
Disadvantage:<br>
- more boiler-plate code<br>
<br>
Advantage:<br>
+ speed<br>
+ avoid coder mistakes<br><div class="im"></div></blockquote><div><br>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.<br>
<br>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. <br>
Callgrind shows 5 properties that are read 465 times while painting, all other properties are already stored in the spray brush.<br><br>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.<br>
<br>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.<br><br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="im">
><br>
> > Problems:<br>
> > * It is very frustrating job to port my paintops as I have many options<br>
> > in GUI. It is easy to make mistake because of the code like<br>
> > getInt("Spray/diaemter") compiles fine, but it has mistake (it suppose to<br>
> > be<br>
> > diameter). Is there any nice way to fprint the KisPaintOpSettings?<br>
><br>
> I know it's a bit frustrating with all those settings, but for the presets<br>
> to work everythings must be serializable.<br>
> The settings have a method dump() that outputs all properties. To avoid<br>
> mistakes you can either define some strings and use these instead of<br>
> hardcoding the strings or write a unittest that check the roundtrip of all<br>
> properties.<br>
<br>
</div>Yes, then I discovered dump(), I changed it to const.<br>
Does the KisPaintOpSettings still has to be const object? Why?</blockquote><div> </div><div>Does it need to be non-const? Not for dump at least.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">
> > * I wanted to return QImage in the settings object because spray uses<br>
> > QImage.But there isn't method getQImage("Spray/imageBrush") or something<br>
> > like<br>
> > this.<br>
><br>
> You can still put the QImage as a member in the sprayop settings.<br>
> Everythings that isn't a property won't be saved automatically, so you have<br>
> to do the saving code yourself. For a QImage the filename of the source<br>
> file could be added as property additionally to the QImage.<br>
><br>
> Another possibility would be to use a pattern from the pattern server<br>
> instead of QImage. That way you only have to store the id of the pattern<br>
> and the paintop could fetch it from the server.<br>
><br>
> There is also the possibility that the image can't be loaded from disk when<br>
> the preset is loaded, because it might be removed or something like that.<br>
> For that I added a method canPaint() to KisPaintOp that can check if the<br>
> settings are read correctly and block painting in case e.g. the image is<br>
> missing.<br>
<br>
</div>KisSprayPaintOpSettings inherits from KisPropertiesConfiguration so I cast the<br>
setting object in writeOptionSetting and call setting->setImage(QImage) for<br>
now.<br>
<br>
I would like to develop some nice widget for selecting bitmap brushes,<br>
something like the one for Gimp brushes, maybe the same, but I would like to<br>
add some other features like [x] "Compute the center of the brush".<br>
Maybe add this for discussion with Peter :)<br></blockquote></div><br>We can have a discussion at the sprint about that. I'm sure there will be some input from the usability discussion too.<br>