Clips and masks fail to update in live effects

This patch solve the problem.

@Jabiertxof - could you attach a sample SVG file which demonstrates the reported bug, and can used to test your patch (ideally with a screenshot on how it is supposed to render)?

I'm not sure what to expect, and if I test the patch with path effects (Bend, Envelope) applied to a clipped path, or a group with a clipped path inside, there is no difference to trunk (the clip-path is not transformed based on the path effect applied to the clipped path).

Hi,~suv, realy there are two bugs:
1-live effects dont work on gruped shapes, ungruped, for example bend work.
2-Live effects dont take the value of clip and mask objects, this patch fix that, but remember need be pats not rectangles or stars...
When have time i attempt to fix bug 1.
For the second i atach a svg whith 4 clips. Use, for example the bend LPE whith a release pached or not.
After i put 2 screenshots.

bug for first one: https://bugs.launchpad.net/inkscape/+bug/1242170

> Hi,~suv, realy there are two bugs:
> 1-live effects dont work on gruped shapes, ungruped, for example bend work.

I didn't use shapes for my earlier test (I do know from experience that they do not work inside groups if path effects are applied to the group itself, and was careful to only use paths).

Thanks for attaching the files …

Seems that the patch only works for clipping applied to a group, not to an individual path (e.g. inside a group which has a Bend path effect)?

~suv is your clip or mask a shape or a path?

ok i dont know could be paths clipped...

Whith this patch, the live effects is preformed to cliped or masked paths or groups.
Requisites:
Paths want to LPE need not Shapes because https://bugs.launchpad.net/inkscape/+bug/1242170
Todo:
This apply only if the LPE object is a group, to do make for not root gruped element. The root could or not be cliped or masked.

Whith this patch. live effects are applied to clips and mask in:
Groups
Paths
Star
Spiral
3dPlane
Ellipse

Attaching sample file where clips on transformed (translated) groups doesn't work well with e.g. Envelope LPE.
(tested with r12763 + live_effects_with_clips_and_mask_V3.diff on OS X 10.7.5)

Possibly this is due to an existing problem with clips/masked and preserved transforms on groups, and needs to be fixed elsewhere.

This patch solve the problem whith nested transformed groups, point by su_v

File from comment 14 seems to render well (wista 64 bit) with patch from comment 15.
However,
1 when entering the group at the bottom right (double click),
2 clicking on the red "squares" so that group #g4100 is selected
3 and releasing the clip (right click and hitting the option from the drop down menu)

No entry appears in the history. Hitting CTRL-Z does something weird to the rendering ... (possibly a regression or this already described?)

Hi Kris, I see the problem, thanks for report

Hi Kris, I test the problem with a fresh branch and the same file.
I dont know why, but some times history write and sometimes not, specialy when revert to saved.
And the problem still here in the fresh branch. Steps to reproduce:
1.open the file
2. create a, for example a square
3 entering the group at the bottom right (double click),
4 clicking on the red "squares" so that group #g4100 is selected
5 releasing the clip
6 2 ctrl-z

The group at the bottom right reduce itself.

I open a new bug here: https://bugs.launchpad.net/inkscape/+bug/1268554

It happens only if the release action isn`t in the first place in history.
The revert one is a new bug: https://bugs.launchpad.net/inkscape/+bug/1268560

Hi Jabier,
  Your mail provider is blocking mine, so I'll write my quick answer here:

Hi Jabiertxo,
 quick reply, I have not yet look at your new diff.

I think it is much better if

apply_lpe_to_mask

and

apply_lpe_to_clippath

are *not* member functions. They are not part of the class logically, so should not be in there. (note: in C++ it is not the case that every function should be in a class, on the contrary it is often much better if they are not in the class definition!)

On 14-1-2014 10:41, Jabiertxo Arraiza Cenoz wrote:
> Hi Johan.
> First thanks for your time, i comment the code a few:
> Not sure is ok, but finaly, i create two functions for handle most of
> duplicate content. Both are in sp-clippath.cpp and in sp-mask.cpp, the
> functions are very similar and maybe is beter use only one in, for
> example sp-path.cpp.
>
> Another cuestion is this duplicate code:
> ===================================
> + if(SP_IS_MASK(SP_ITEM(this)->mask_ref->getObject())){
> + SP_ITEM(this)->mask_ref->getObject()->apply_lpe_to_mask(this);
> + }
> + if(SP_IS_CLIPPATH(SP_ITEM(this)->clip_ref->getObject())){
> + SP_ITEM(this)->clip_ref->getObject()->apply_lpe_to_clippath(this);
> + }
> ===================================
> It can be changed for one or two function/s call/s
>
> And for the last, a silly cuestion, Whats better?
> This:
> ===================================
> if(SP_IS_MASK(SP_ITEM(this)->mask_ref->getObject())){
> SP_ITEM(this)->mask_ref->getObject()->apply_lpe_to_mask(this);
> }
> ===================================
> or this:
> ===================================
> SPMask mask = NULL; //is good declare it null first?
> mask = SP_ITEM(this)->mask_ref->getObject();
> if(SP_IS_MASK(mask)){
> mask->apply_lpe_to_mask(this);
> }
> ===================================

Best is:

SPMask *mask = SP_ITEM(this)->mask_ref->getObject();
if(SP_IS_MASK(mask)){
    apply_lpe_to_mask(mask,...);
}

regards,
  Johan

Hi Johan, sorry for the problem, we have a double-bounce configuration
in our mail server and perhaps is for that, need to see the logs...

This weekend change the code and send with an alternate mail.

Good weekend.

El mié, 15-01-2014 a las 22:26 +0000, Johan Engelen escribió:
> Hi Jabier,
> Your mail provider is blocking mine, so I'll write my quick answer here:
>
> Hi Jabiertxo,
> quick reply, I have not yet look at your new diff.
>
> I think it is much better if
>
> apply_lpe_to_mask
>
> and
>
> apply_lpe_to_clippath
>
> are *not* member functions. They are not part of the class logically, so
> should not be in there. (note: in C++ it is not the case that every
> function should be in a class, on the contrary it is often much better
> if they are not in the class definition!)
>
>
> On 14-1-2014 10:41, Jabiertxo Arraiza Cenoz wrote:
> > Hi Johan.
> > First thanks for your time, i comment the code a few:
> > Not sure is ok, but finaly, i create two functions for handle most of
> > duplicate content. Both are in sp-clippath.cpp and in sp-mask.cpp, the
> > functions are very similar and maybe is beter use only one in, for
> > example sp-path.cpp.
> >
> > Another cuestion is this duplicate code:
> > ===================================
> > + if(SP_IS_MASK(SP_ITEM(this)->mask_ref->getObject())){
> > + SP_ITEM(this)->mask_ref->getObject()->apply_lpe_to_mask(this);
> > + }
> > + if(SP_IS_CLIPPATH(SP_ITEM(this)->clip_ref->getObject())){
> > + SP_ITEM(this)->clip_ref->getObject()->apply_lpe_to_clippath(this);
> > + }
> > ===================================
> > It can be changed for one or two function/s call/s
> >
> > And for the last, a silly cuestion, Whats better?
> > This:
> > ===================================
> > if(SP_IS_MASK(SP_ITEM(this)->mask_ref->getObject())){
> > SP_ITEM(this)->mask_ref->getObject()->apply_lpe_to_mask(this);
> > }
> > ===================================
> > or this:
> > ===================================
> > SPMask mask = NULL; //is good declare it null first?
> > mask = SP_ITEM(this)->mask_ref->getObject();
> > if(SP_IS_MASK(mask)){
> > mask->apply_lpe_to_mask(this);
> > }
> > ===================================
>
> Best is:
>
> SPMask *mask = SP_ITEM(this)->mask_ref->getObject();
> if(SP_IS_MASK(mask)){
> apply_lpe_to_mask(mask,...);
> }
>
>
> regards,
> Johan
>

The code improved with the big help of Johan Engelen. I atemp to contact with him using my gmail account instead my main account, because a double bounce problems. Think Johan get it but if not, here is the last patch.

Thanks Johan.

Some improvements by Johan Engelen.

Fixes a 3DBox problem patching

This new pach fixed the bug pointed by Josh Andler.

Jabier, please do a thorough check of all your changes. I found a very obvious bug in your diff, which makes me worried the code has not been tested.
In 'src/selection-chemistry.cpp':
    copy->setAttribute(copy->attribute("inkscape:original-d"),"d");
must be a bug. It should read
    copy->setAttribute("d", copy->attribute("inkscape:original-d"));

Did you test that piece of code?

Hi Johan, you are correct!
Sorry for don't test the release clip fix correctly. :(
Regards, Jabier.

Jabier, why is there code like
+ SPMask * mask = SP_ITEM(lpeitem)->mask_ref->getObject();
+ if(SP_IS_MASK(mask)){
instead of
+ SPMask * mask = SP_ITEM(lpeitem)->mask_ref->getObject();
+ if (mask) {
?
This seems unnecessary. (the SP_xxx functions can be costly, and if they are not needed, you shouldn't use them).

Please also not proper code formatting, space between if and (, between ) and {, etc.
You can use clang-format to help you.

Actually, make that:
+ SPMask * mask = lpeitem->mask_ref->getObject();
+ if (mask) {

You should try to eliminate as many casts as possible. Very often, you don't need one.

Update the patch with Johan indications. Now also handle groups in clips or mask

There are still a couple of casts left.

+sp_gslist_update_by_clip_or_mask(GSList *item_list,SPItem * item)
+{
+ if(SP_IS_MASK(item->mask_ref->getObject())) { <-- unnecessary
...
+ if(SP_IS_CLIPPATH(item->clip_ref->getObject())) { <-- here too

+sp_lpe_item_apply_to_mask(SPItem * item)
+{
+ SPMask *mask = item->mask_ref->getObject();
+ if(SP_IS_MASK(mask)) { <--- not needed

Fixed, thanks Johan for the patience.

Code looks good now Jabier, please commit.

Thanks for your review! I commit first onto experimental

Hi to all.
This patch is because depend the type of live effect could be ok or wrong, apply this effect to clips and mask.

For example BEND and ENVELOPE, LATTICE... are effects give improved results with clips/mask
SKETCH, HATCHES... give wrong results.

With this patch you can active it in each effect.
For now is only applied to Bend LPE for testing.

To enable more effects, look arround line added to code in lpe-bendpath.cpp
TODO: Make a full list of effects handle well with LPE on clips/mask.

Regards, Jabier.

Reopening. The patched behavior is not desired for LPE Sketch and others. A different solution should be found, perhaps a toggle switch for the user to choose between applying the effect to clip/mask or not.

Hi again with this bug.
I add two pach one for experimental and another for trunk -next entry-
Finaly i find a way too much simple than the original. All code reside in the tree modified functions in sp-lpe-item files.
To use in one particular effet you need to put in the "doBeforeEffect" function this code:

SPLPEItem * item = const_cast<SPLPEItem*>(lpeitem);
item->apply_to_clippath(item);
item->apply_to_mask(item);

After this, the modified effect apply also to clip and mask.

I do a pre-selection to suitable effect for apply it, and apply to them in this patchs.

For trunk i add to this effects:
envelope deformation
bend path
lattice deformation
mirror symmetry
offset
perspective_path
vonKoch

And for experimental to this:
envelope deformation
bend path
lattice deformation
mirror symmetry
offset
perspective_path
vonKoch
roughen
simplify
perspective/envelope
lattice2

Patch for trunk

patch for experimental

The experimental branch was merged into trunk in r13641 - jabiertxof, could you update on the current status, and which patches still need to be applied to trunk?

~suv the patch is applied in experimental, so nothing to do about this bug.But im compiling trunk to a real try.

Confirmed patch correctly applied to 0.92.