[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Filter effects for karbon
From:       Thomas Zander <zander () kde ! org>
Date:       2009-04-14 17:51:07
Message-ID: 200904141951.07668.zander () kde ! org
[Download RAW message or body]

On Monday 13 April 2009 21:58:40 Cyrille Berger wrote:
> > On 2009-04-13 05:08:35, Thomas Zander wrote:
> > > trunk/koffice/libs/flake/KoShape.h, line 779
> > > <http://reviewboard.kde.org/r/580/diff/1/?file=5252#file5252line779>
> > >
> > >     You should not return a reference to a QList (or any QSharedData
> > > class) as it exposes the internals and will cause crashes if the caller
> > > keeps a reference to the list around when the shape is deleted.
>
> So instead, for the non-const, I should have
> insertFilterEffect(KoFilterEffect* newFilter, KoFilterEffect* aboveThisOne
> ); and removeFilterEffect(KoFilterEffect* filter); ?

What about mirroring QList API; insert(int index, KoFilterEffect*); and 
removeAll() removeAt() as well.

> > On 2009-04-13 05:08:35, Thomas Zander wrote:
> > > trunk/koffice/libs/flake/KoFilterEffect.h, line 45
> > > <http://reviewboard.kde.org/r/580/diff/1/?file=5250#file5250line45>
> > >
> > >     The API is a bit unclear; I don't know what 'needed' is and I am
> > > confused why you need to pass something called 'needed' to a method
> > > called 'needed' to get back the 'needed' rect.  This gives me circular
> > > dependencies ;)
>
> needed/changed are a bit difficult to explain without an example. The best
> example is the convolution filter (for instance blur), lets take a small
> convolution with a 3x3 kernel, lets say we have a to filter a 10x10 area,
> to compute each pixel you need to access all the surrounding pixel, which
> means you need the value of a border of one pixel surrounding the square,
> and in this case, you will return a rect of 12x12, and indeed the parameter
> should be named 'inputRect'.
>
> changed is the other way around, lets say I have changed the Rect(4,4, 8,
> 8), the function changed should answer which pixels are affected by that
> change ? I am not yet sure if "changed" is useful in the context of flake,
> I am not sure we have a way to know which pixels have been changed and need
> refiltering. While needed is well... needed :D

Hmm, your example is a bit too academic for me to grasp and I have a hard time 
fitting this in the context of flake. Why did you add this API? You are 
currently not using it and I'm wondering who would use these calls.

I do remember one problem that you may be thinking of and why you introduced 
these methods.  Maybe you can confirm;
if I have an flake shape that is 100x100 pixels in size and I then blur it then 
properly speaking I should get an image that is 104x104 pixels back from the 
filter since the blurring makes it bigger.
Is that the problem you are trying to solve?  How do these methods help you do 
that? Are they supposed to be called from the shapeManager?

> > On 2009-04-13 05:08:35, Thomas Zander wrote:
> > > trunk/koffice/libs/flake/KoFilterEffect.h, line 35
> > > <http://reviewboard.kde.org/r/580/diff/1/?file=5250#file5250line35>
> > >
> > >     Why pass in an id and name if its never used?
>
> Will be used to identify filters. The name will be used in the UI, while
> the ID will be the key in the registry. 

Then I suggest following the shape/tool API and make it setters. Like 
KoTool::setToolId() which is quite helpful in handling buggy (3rd party) 
plugins.
Having a name on the filter can be avoided if we introduce factories anyway 
(required for plugins) so you only have to have the translated name one time; 
on the factory. This makes most sense if we end up adding more information, 
like a tooltip etc. Saves us from the need to change the constructor of an 
interface for extending functionality like that ;)

> The API of KoFilterEffect isn't
> finalized, there is a need for two functions for saving/loading parameters,
> as well as a function returning a widget for configuration. 

All probably should go on a factory if you are thinking about making this a 
plugin structure.
See the tool/docker/shape factories for examples of how this is done in other 
koffice libs classes.

> And we have to
> think about what parameters are needed for 'processImage', at least the
> zoom, since we don't want the bluring effect to change when the user zoom,

Having a KoViewConverter parameter is probably best for solving the zoom 
issues ;)

> and there is also probably a need to give a QRect(F) which tell which part
> of the shape correspond to the image (especially useful if we do chunk
> filtering and want to support feDisplacementMap). 

Makes sense.

> Actually, to be future
> proof, a good idea could be to pass to processImage a
> KoFilterEffect::ProcessInformation that could be more easilly extended with
> information in the future (any similarity with
> http://www.koffice.org/developer/apidocs/krita-image/classKisProcessingInfo
>rmation.html is probably due to the fact that Krita developers stole all my
> ideas ;p )

Hehe, *cough* QStyleOption *cough*

To be honest, I don't really see the advantage of that, we need to alter all 
filters anyway in case we add an option there since any addition will most 
likely not have the same behavior if the filter ignores it.
And more importantly, this solution makes for a much harder to grasp API for 
the filter writers.

And to be blunt, I am kind of hoping that various years of Krita experience 
would allow you guys to come up with a stable API.  Lets just take our time 
before we freeze the API.

> I also think the KoFilterEffect API needs to remain private for the time
> being, I still want to think if there is a way to have more code sharing
> between flake/FilterEffect and Krita/Filter

Yeah, that brings forward a bit of a sticky issue; many people would love to 
use certain flake shapes we make.  And *just* making those people require the 
flake libs would be fine, its not that big.  But if that pulls in more libraries 
than one or two, I fear people will try to stay away from useful ODF features 
we have just because of that.
With the introduction of libodf or similar I was hoping we could make our 
library and its dependencies smaller, not bigger.  So we can tell the mathml 
users that they can use the flake shape rather cheaply.

So if we manage to share filters, thats great.  Just watch the dependencies, 
please.

Cheers!
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]