Fixed bug in KisPerChannelFilter that prevented it from loading/saving bookmarks v.3

[Cyrille Berger]

On Monday 09 March 2009, Dmitry Kazakov wrote:
> >  > The first issue is toXML/toLegacyXML stuff. These functions are
> >
> > implemented
> >
> > > in _different_ classes although they do the same things. More than
> > > that, both of them are called from different parts of krita: bookmark
> > > manager uses toXML, kis_kra_ uses toLegacyXML Why not have the only
> > > one? The only united interface?
> >
> > .kra file are expected to use the toLegacyXML because this functions
> > saves to an XML formated for the 1.6 .kra files. While toXML use the
> > OpenRaster style, described in
> > http://create.freedesktop.org/wiki/index.php?title=OpenRaster/Layers_Stac
> >k_Specification.
>
> Do you mean filter's configuration should look like that?
>
> <filter name="perchannel" type="applications:Krita:PerChannel">
>      <params version=1>
>        <param name="CurvesNumber">3</param>
>        <param name="curve0">0,0;0.5,0.5;1,1;</param>
>        <param name="curve1">0,0;1,1;</param>
>        <param name="curve2">0,0;1,1;</param>
>      </params>
> </filter>

Yes.

>
>  even if it will be very inspired by the SVG spec.
>
>
> I'm afraid we can't be inspired by SVGspec here at all. The point is they
> suggest us to use linear interpolation for the curve (i guess this type of
> the curve had been implemented in KCurve before my patch) . Maybe this type
> is a good idea for some ideal theoretical curve as it's quite simple and
> fast, but for real users (who are "inspired" by real professional
> instrumens, such as PS, PictureWindowPro, TheGimp) it's at least _strange_
> curve and almost unusable, because they are used to cubic splines.

Difference between inspiration and copy/paste is that you can make changes ;) I 
have no objection to adding cubic splines to the list of types, and to 
properly specify it.

> Of course, we can interpolate cubic spline, drawn by the user, to the
> linear one and write the latter to the file in SVG format. But the user
> won't be able to change the curve later.
>
> And the last issue against SVG. SVG's feComponentTransferis is stuck to
> RGB. RGB is not the only colorspace in the world. ;)

Well you have shown one of the solution to that problem in the beginning of 
the mail ;) While I am not a fan of numbers, it might as well be better to 
have curve_red, curve_blue, curve_cyan, curve_l, curve_...

> > > KisBaseProcessor::defaultConfiguration(const KisPaintDeviceSP pd)
> >
> > The documentation says all.
>
> Where can i take a look on this documentation?

In the header, or http://www.koffice.org/developer/apidocs/ (more specificly 
http://www.koffice.org/developer/apidocs/krita-
image/classKisBaseProcessor.html#09a63a0fbbf406ccddd0409fe2644e77 )

> > It's the configuration that will be used in the filter dialog when the
> > user start the dialog.
> >
> > You forgot that way as well:
> > KisFilterConfiguration * KisBaseProcessor::factoryConfiguration(const
> > KisPaintDeviceSP) const
> > whose name sucks and is a bit confusing (if anyone has a better idea,
>
> createConfiguration?
I think it's a too generic name.

> > Filter configurations are saved in KisBookmarkedConfigurationManager
> > which is a generic class which can be used with all kind of
> > KisSerializableConfiguration to store them, this can be
> > filters/generators configuration, paintop settings, ... The only way
> > KisBookmarkedConfigurationManager can create the correct subclass of
> > KisSerializableConfiguration is by using the
> > KisFilterConfigurationFactory.
>
> The problem is that KisKraLoader::loadAdjustmentLayer calls
> KisBaseProcessor::defaultConfiguration(...). The latter one calls
> factoryConfiguration and (if factoryConfiguration isn't overridden in
> KisPerChannelFilter) creates KisFilterConfiguration instead of needed
> KisPerChannelFilterConfiguration. This causes adjustment layers not being
> loaded properly from .kra.
>
> The easiest (and the most illogical) workaround is to change
> KisBookmarkedConfigurationManager::defaultConfiguration, that has access to
> filter's configurationFactory, to call factory's createDefault if nothing
> can be loaded from kritarc.

Why not provide an implementation of KisBaseProcessor::factoryConfiguration in  
KisPerChannelFilter ?

> KisSerializableConfiguration*
> KisBookmarkedConfigurationManager::defaultConfiguration() const
> {
>     if (exists(KisBookmarkedConfigurationManager::ConfigDefault.id())) {
>         return load(KisBookmarkedConfigurationManager::ConfigDefault.id());
>     }
> ///skipped///
>     return d->configFactory->createDefault();
> }
>
> In this case we won't need factoryDefault at all. But this change is _very
> bad idea_ as it's illogical.

Yes. Not good.

> In my opinion Factories should somehow call Filter's defaultConfiguration
> instead of having their own createDefault and create(...). In this case,
> maybe, we won't even be needed to override KisConfigurationFactory for
> every filter.

Except defaultConfiguration it will break the purpose of defaultConfiguration, 
and will trigger an endless loop, since KisFilter::defaultConfiguration() call 
the BookmarkManager to get the configuration which will then call the factory. 
But it might be a good idea to use KisBaseProcessor::factoryConfiguration for 
that, instead.
The main advantages, would be that there would, indeed, be no more need for 
overriding KisConfigurationFactory, and if the filter get new parameters they 
are loaded from the factoryConfiguration. And I can't see a drawback.

-- 
Cyrille Berger
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20090309/e5e10298/attachment-0001.htm