[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: Re: Review Request: Filter effects for karbon
From: Cyrille Berger <cberger () cberger ! net>
Date: 2009-04-25 19:56:16
Message-ID: 200904252156.17166.cberger () cberger ! net
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Tuesday 14 April 2009, Thomas Zander wrote:
> 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?
yes.
> How do these methods help you
> do that?
You call changed(100x100) and it returns you a 104x104 pixels. needed is
probably not needed for flake, it's only usefull for partial
> Are they supposed to be called from the shapeManager?
yes. There is a todo :)
I think it would probably best I implement one of the filter that need that, it
will makes things more clear.
> > > 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 ;)
Not sure, because the filter name would need to be used in uservisible
situation (like the docker of filter affected to a given shape), and depending
on how we do it UI wise, each filter might need a different name (like in
inkscape)
> > 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.
yup. But I am not sure if it should be made plugin now (or at least public
plugin, I would like the API to remain koffice-private for 2.1, especially if
flake reach API/ABI stability in 2.1 ).
> > 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/classKisProcessingIn
> >fo rmation.html is probably due to the fact that Krita developers stole
> > all my ideas ;p )
>
> Hehe, *cough* QStyleOption *cough*
hum no :) There is a difference, with QStyleMessErmOption :) the style is
expected to act upon all the parameters, while here the ProcessInformation is
information that the filter can use if it wants. For instace, the invert filter
don't care about anything, it just take the pixel in entry and invert it. The
blur filter doesn't care the rectangle and only the zoom.
> 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.
No we don't. We only need to add this in case a filter need more information.
> 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.
Well five (or is it six ?) years of Krita experience taught me that filters are
very creative at finding other information they want.
That said, the svg:filter spec is rather limited, and if we want to limit
ourself to it (but I don't see why), the KoViewConverter is good enough
(doesn't even seem the QRect is needed... only to give as input the proper
coordinate of the current filtering).
> > 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.
No new dependency for flake :) But in plugins. Or just an interface, but then
again, might as well just be in plugin, and since krita already depends on
flake, the interface(s) could be in flake. And the user of the library would
only have those filters when krita is installed (well if you want some
features, at some point you need to have code installed, hein ;p )
--
Cyrille Berger
[Attachment #5 (text/html)]
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" \
"http://www.w3.org/TR/REC-html40/strict.dtd"><html><head><meta name="qrichtext" \
content="1" /><style type="text/css">p, li { white-space: pre-wrap; \
}</style></head><body style=" font-family:'DejaVu Sans'; font-size:9pt; \
font-weight:400; font-style:normal;">On Tuesday 14 April 2009, Thomas Zander \
wrote:<br> > On Monday 13 April 2009 21:58:40 Cyrille Berger wrote:<br>
> > > On 2009-04-13 05:08:35, Thomas Zander wrote:<br>
> > > > trunk/koffice/libs/flake/KoShape.h, line 779<br>
> > > > <http://reviewboard.kde.org/r/580/diff/1/?file=5252#file5252line779><br>
> > > ><br>
> > > > You should not return a reference to a QList (or any \
QSharedData<br> > > > > class) as it exposes the internals and will cause \
crashes if the<br> > > > > caller keeps a reference to the list around \
when the shape is<br> > > > > deleted.<br>
> ><br>
> > So instead, for the non-const, I should have<br>
> > insertFilterEffect(KoFilterEffect* newFilter, KoFilterEffect*<br>
> > aboveThisOne ); and removeFilterEffect(KoFilterEffect* filter); ?<br>
><br>
> What about mirroring QList API; insert(int index, KoFilterEffect*); and<br>
> removeAll() removeAt() as well.<br>
><br>
> > > On 2009-04-13 05:08:35, Thomas Zander wrote:<br>
> > > > trunk/koffice/libs/flake/KoFilterEffect.h, line 45<br>
> > > > <http://reviewboard.kde.org/r/580/diff/1/?file=5250#file5250line45><br>
> > > ><br>
> > > > The API is a bit unclear; I don't know what 'needed' is and I \
am<br> > > > > confused why you need to pass something called 'needed' to \
a method<br> > > > > called 'needed' to get back the 'needed' rect. This \
gives me<br> > > > > circular dependencies ;)<br>
> ><br>
> > needed/changed are a bit difficult to explain without an example. The<br>
> > best example is the convolution filter (for instance blur), lets take a<br>
> > small convolution with a 3x3 kernel, lets say we have a to filter a \
10x10<br> > > area, to compute each pixel you need to access all the \
surrounding pixel,<br> > > which means you need the value of a border of one \
pixel surrounding the<br> > > square, and in this case, you will return a rect \
of 12x12, and indeed the<br> > > parameter should be named 'inputRect'.<br>
> ><br>
> > changed is the other way around, lets say I have changed the Rect(4,4, \
8,<br> > > 8), the function changed should answer which pixels are affected by \
that<br> > > change ? I am not yet sure if "changed" is useful in the context \
of<br> > > flake, I am not sure we have a way to know which pixels have been \
changed<br> > > and need refiltering. While needed is well... needed :D<br>
><br>
> Hmm, your example is a bit too academic for me to grasp and I have a hard<br>
> time fitting this in the context of flake. Why did you add this API? You<br>
> are currently not using it and I'm wondering who would use these calls.<br>
><br>
> I do remember one problem that you may be thinking of and why you<br>
> introduced these methods. Maybe you can confirm;<br>
> if I have an flake shape that is 100x100 pixels in size and I then blur it<br>
> then properly speaking I should get an image that is 104x104 pixels back<br>
> from the filter since the blurring makes it bigger.<br>
> Is that the problem you are trying to solve?<br>
yes.<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>> How do these methods help you<br> > do that? <br>
You call changed(100x100) and it returns you a 104x104 pixels. needed is probably not \
needed for flake, it's only usefull for partial<br> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>> Are they supposed to be called from the \
shapeManager?<br> yes. There is a todo :)<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>I think it would probably best I implement one of the \
filter that need that, it will makes things more clear.<br> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>> > > On 2009-04-13 05:08:35, Thomas Zander \
wrote:<br> > > > > trunk/koffice/libs/flake/KoFilterEffect.h, line 35<br>
> > > > <http://reviewboard.kde.org/r/580/diff/1/?file=5250#file5250line35><br>
> > > ><br>
> > > > Why pass in an id and name if its never used?<br>
> ><br>
> > Will be used to identify filters. The name will be used in the UI, \
while<br> > > the ID will be the key in the registry.<br>
><br>
> Then I suggest following the shape/tool API and make it setters. Like<br>
> KoTool::setToolId() which is quite helpful in handling buggy (3rd party)<br>
> plugins.<br>
> Having a name on the filter can be avoided if we introduce factories anyway<br>
> (required for plugins) so you only have to have the translated name one<br>
> time; on the factory. This makes most sense if we end up adding more<br>
> information, like a tooltip etc. Saves us from the need to change the<br>
> constructor of an interface for extending functionality like that ;)<br>
<p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>Not sure, because the filter name would need to be used in \
uservisible situation (like the docker of filter affected to a given shape), and \
depending on how we do it UI wise, each filter might need a different name (like in \
inkscape)<br> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>> > The API of KoFilterEffect isn't<br> > > \
finalized, there is a need for two functions for saving/loading<br> > > \
parameters, as well as a function returning a widget for configuration.<br> ><br>
> All probably should go on a factory if you are thinking about making this a<br>
> plugin structure.<br>
yup. But I am not sure if it should be made plugin now (or at least public plugin, I \
would like the API to remain koffice-private for 2.1, especially if flake reach \
API/ABI stability in 2.1 ).<br> <p style="-qt-paragraph-type:empty; margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;"><br></p>> > And we have to<br> > > \
think about what parameters are needed for 'processImage', at least the<br> > > \
zoom, since we don't want the bluring effect to change when the user<br> > > \
zoom,<br> ><br>
> Having a KoViewConverter parameter is probably best for solving the zoom<br>
> issues ;)<br>
><br>
> > and there is also probably a need to give a QRect(F) which tell which<br>
> > part of the shape correspond to the image (especially useful if we do<br>
> > chunk filtering and want to support feDisplacementMap).<br>
><br>
> Makes sense.<br>
><br>
> > Actually, to be future<br>
> > proof, a good idea could be to pass to processImage a<br>
> > KoFilterEffect::ProcessInformation that could be more easilly extended<br>
> > with information in the future (any similarity with<br>
> > http://www.koffice.org/developer/apidocs/krita-image/classKisProcessingIn<br>
> >fo rmation.html is probably due to the fact that Krita developers stole<br>
> > all my ideas ;p )<br>
><br>
> Hehe, *cough* QStyleOption *cough*<br>
hum no :) There is a difference, with QStyleMessErmOption :) the style is expected to \
act upon all the parameters, while here the ProcessInformation is information that \
the filter can use if it wants. For instace, the invert filter don't care about \
anything, it just take the pixel in entry and invert it. The blur filter doesn't care \
the rectangle and only the zoom.<br> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>> To be honest, I \
don't really see the advantage of that, we need to alter<br> > all filters anyway \
in case we add an option there since any addition will<br> > most likely not have \
the same behavior if the filter ignores it.<br> No we don't. We only need to add this \
in case a filter need more information.<br> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>> And more \
importantly, this solution makes for a much harder to grasp API<br> > for the \
filter writers.<br> ><br>
> And to be blunt, I am kind of hoping that various years of Krita experience<br>
> would allow you guys to come up with a stable API. Lets just take our time<br>
> before we freeze the API.<br>
Well five (or is it six ?) years of Krita experience taught me that filters are very \
creative at finding other information they want.<br> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"><br></p>That said, the svg:filter spec is rather limited, and if \
we want to limit ourself to it (but I don't see why), the KoViewConverter is good \
enough (doesn't even seem the QRect is needed... only to give as input the proper \
coordinate of the current filtering).<br> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>> > I also \
think the KoFilterEffect API needs to remain private for the time<br> > > \
being, I still want to think if there is a way to have more code sharing<br> > \
> between flake/FilterEffect and Krita/Filter<br> ><br>
> Yeah, that brings forward a bit of a sticky issue; many people would love<br>
> to use certain flake shapes we make. And *just* making those people<br>
> require the flake libs would be fine, its not that big. But if that pulls<br>
> in more libraries than one or two, I fear people will try to stay away from<br>
> useful ODF features we have just because of that.<br>
> With the introduction of libodf or similar I was hoping we could make our<br>
> library and its dependencies smaller, not bigger. So we can tell the<br>
> mathml users that they can use the flake shape rather cheaply.<br>
No new dependency for flake :) But in plugins. Or just an interface, but then again, \
might as well just be in plugin, and since krita already depends on flake, the \
interface(s) could be in flake. And the user of the library would only have those \
filters when krita is installed (well if you want some features, at some point you \
need to have code installed, hein ;p )<br> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"><br></p>-- <br> Cyrille \
Berger</p></body></html>
_______________________________________________
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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic