are selections part of kispaintdevice or aren't they?

That's the question...

Right now they are, but that gives a problem with the
following bit of code 
from KisFilterMask, especially when filters take the
selection from the paint 
device during KisFilter::process()

void KisFilterMask::apply( KisPaintDeviceSP projection,
const QRect & rc ) 
const
{
    Q_ASSERT( m_d->filterConfig );
    if (!m_d->filterConfig) return;

    selection()->updateProjection();

    // XXX: This is actually a problem when we're running
multithreaded, so we 
need to make
    // selection a paramenter of KisFilter::process
    KisSelectionSP oldSelection = 0;
    if (projection->hasSelection())
        oldSelection = projection->selection();

    projection->setSelection( selection() );

    KisFilterSP filter =
KisFilterRegistry::instance()->value( 
m_d->filterConfig->name() );
    if (!filter) {
        kWarning() << "Could not retrieve filter
with name " <<  
m_d->filterConfig->name();
        return;
    }

    filter->process( projection, rc,
m_d->filterConfig);

    projection->setSelection( oldSelection );
}

If a layer has more than one filter mask, and if we split up
a recomposition 
task in chunks, then it is very likely that there will be
more than one 
KisFilterMask active on the same projection paint device.
Which means that 
they change the selection while another iterator from
another filter is 
accessing them.

Poof!

My feeling is that the only solution is to take KisSelection
away from 
KisPaintDevice, but that means that the iterator's
isSelected methods no 
longer work and that we have to manually create an iterator
on a selection 
every time we access a paint device.

Any ideas?
-- 
Boudewijn Rempt 
http://www.va
ldyas.org/fading/index.cgi

_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

On Thursday 04 October 2007, Boudewijn Rempt wrote:
> My feeling is that the only solution is to take
KisSelection away from
> KisPaintDevice, but that means that the iterator's
isSelected methods no
> longer work and that we have to manually create an
iterator on a selection
> every time we access a paint device.
>
> Any ideas?
Wondering about that. A possibility is to give the
KisSelection as a 
createIterator/createRandomAccessor parameter. The question
is wether it's 
only laziness on our part (less code change, but no shame to
that 
) or 
does it also make sense API wise (and do we care enought ?)
?

-- 
Cyrille Berger
_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

On Thursday 04 October 2007, Cyrille Berger wrote:
> On Thursday 04 October 2007, Boudewijn Rempt wrote:
> > My feeling is that the only solution is to take
KisSelection away from
> > KisPaintDevice, but that means that the iterator's
isSelected methods no
> > longer work and that we have to manually create an
iterator on a
> > selection every time we access a paint device.
> >
> > Any ideas?
>
> Wondering about that. A possibility is to give the
KisSelection as a
> createIterator/createRandomAccessor parameter. The
question is wether it's
> only laziness on our part (less code change, but no
shame to that  ) or
> does it also make sense API wise (and do we care
enought ?) ?

I've been sleeping on this. Passing a selection as a
parameter to the iterator 
sounded like a good thing at first, but then I thought, it
would only 
complicate the code. We'd still need ot pass the selection
to the filters and 
everywhere else. I'm not sure whether the convenience of
isSelected() and 
selectedness() compensates for that.

-- 
Boudewijn Rempt 
http://www.va
ldyas.org/fading/index.cgi

_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

Btw, thinking about this. To limit the time spend on
refactoring (yet again) 
the filter. We need to make all the remaining changes in one
go. (it's really 
an ultra boring task, I know about what I am talking).

That means, the switch to the new progress report, that
would allow us to 
remove the protected fields of KisFilter and have process be
a const 
function, and probably be "thread-safe" (at least
at the API level, then 
nothing prevent a nasty filter author to use const_cast).

So the list would be:
 - integrate 
 - all remaining fields of KisFilter moved to the d-pointer
 - constify KisFilter::process
 - the KisSelection change

Am I missing something ?

-- 
Cyrille Berger
_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

On Saturday 06 October 2007, Cyrille Berger wrote:

> So the list would be:
>  - integrate
>  - all remaining fields of KisFilter moved to the
d-pointer
>  - constify KisFilter::process
>  - the KisSelection change
>
> Am I missing something ?

Not that I can see; I am not sure at this moment (it's been
too long since I 
looked at the code) how the progress stuff fits in with
Thomas' progress 
stuff in krita/image. But I think that it's a good idea to
combine these 
items in one refactor operation.

-- 
Boudewijn Rempt 
http://www.va
ldyas.org/fading/index.cgi

_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

On Saturday 06 October 2007, Boudewijn Rempt wrote:
> I've been sleeping on this. Passing a selection as a
parameter to the
> iterator sounded like a good thing at first, but then I
thought, it would
> only complicate the code. We'd still need ot pass the
selection to the
> filters and everywhere else. I'm not sure whether the
convenience of
> isSelected() and selectedness() compensates for that.

I disagree with you, and the more I think about it, the more
I am convinced 
that it is a much better solution than having two
iterators.

I mean, we will have
KisIterator itSrc = srcDevice->createIterator(...);
KisIterator itSrcSelection =
srcSelection->createIterator(...);
KisIterator itDst = dstDevice->createIterator(...);
KisIterator itDstSelection =
dstSelection->createIterator(...);

while(not itSrc.isDone())
{
	...
	++itSrc;
	++itSrcSelection ;
	++itDst;
	++itDstSelection 
}

against :
KisIterator itSrc = srcDevice->createIterator(...,
srcSelection);
KisIterator itDst = dstDevice->createIterator(...,
dstSelection);

while(not itSrc.isDone())
{
	...
	++itSrc;
	++itDst;
}

So no, I do think that having the selection at iterator
level brings more than 
the two function convenience.

But, then for the API of KisFilter::process, we are allready
passing six 
parameters (source device, source origin, destination
device, destination 
origin, size and configuration), and adding selection would
mean two more. I 
think we should consider passing a
KisFilterProcessingInformation structure 
which would contain the device, the origin and the selection
(and would be 
extensible if we found other thing to pass in the future
without suffering 
and editing all filters).

-- 
Cyrille Berger
_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

On Saturday 06 October 2007, Cyrille Berger wrote:
> So no, I do think that having the selection at iterator
level brings more
> than the two function convenience.

Yes, I discussed this on irc with Bart and Casper before you
arrived, and we 
came to the same conclusion. I agree completely with your
proposal.
>
> But, then for the API of KisFilter::process, we are
allready passing six
> parameters (source device, source origin, destination
device, destination
> origin, size and configuration), and adding selection
would mean two more.
> I think we should consider passing a
KisFilterProcessingInformation
> structure which would contain the device, the origin
and the selection (and
> would be extensible if we found other thing to pass in
the future without
> suffering and editing all filters).

Yes, I agree here too.

-- 
Boudewijn Rempt 
http://www.va
ldyas.org/fading/index.cgi

_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

On Saturday 06 October 2007, Boudewijn Rempt wrote:
> Yes, I agree here too.

Great. So I think we should organize a fest-day (or
afternoon) to do this 
together (all person who wants to help are welcome of course
 ) and
also 
porting the remaining filters.

To the list I forgot to mention that some filters need work
on their 
configuration object. I am pretty happy with the bookmarking
of configuration 
(except that saving to KConfig might not be the prettiest
long term solution, 
but that shouldn't affect the filters), so I guess there is
no reason to hold 
off fixing that in the filter.

-- 
Cyrille Berger
_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

On 10/6/07, Boudewijn Rempt <boudvaldyas.org> wrote:
> On Saturday 06 October 2007, Cyrille Berger wrote:
>
> > So the list would be:
> >  - integrate
> >  - all remaining fields of KisFilter moved to the
d-pointer
> >  - constify KisFilter::process
> >  - the KisSelection change
> >
> > Am I missing something ?
>
> Not that I can see; I am not sure at this moment (it's
been too long since I
> looked at the code) how the progress stuff fits in with
Thomas' progress
> stuff in krita/image. But I think that it's a good idea
to combine these
> items in one refactor operation.
KoProgressUpdater is now in guiutils.
_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop

On Saturday 06 October 2007, Cyrille Berger wrote:
> On Saturday 06 October 2007, Boudewijn Rempt wrote:
> > Yes, I agree here too.
>
> Great. So I think we should organize a fest-day (or
afternoon) to do this
> together (all person who wants to help are welcome of
course  ) and
also
> porting the remaining filters.

I'm game -- but first I need to finish the layer/node
thing.

-- 
Boudewijn Rempt 
http://www.va
ldyas.org/fading/index.cgi

_______________________________________________
kimageshop mailing list
kimageshopkde.org
http
s://mail.kde.org/mailman/listinfo/kimageshop