Re: [Inkscape-devel] [PATCH] Grouping when setting mask
On Fri, 2010-03-19 at 16:48 +0100, ~suv wrote:
On 4/2/10 20:59, Joshua A. Andler wrote:
On Wed, 2010-02-03 at 19:10 +0100, Martin Sucha wrote:
Hi all,
I've implemented feature to automatically group objects when setting clip/mask and it now works for me. (Thanks Krzysztof Kosiński for help.)
Could you please review attached patch and commit it if you consider it good enough?
Reviewed and tested, looks good by me (and is something I will use). Committed in bzr rev 9050. Thank you!
Is it possible that this patch introduced a big performance loss when grouping a larger selection of objects? I just stumbled upon this when clipping ~1800 small objects (created with the spray tool) with a rectangle using the new feature to group the selection automatically before applying the clippath to the group: to my surprise this took very long (~45 seconds) whereas grouping the same selection happens almost instantaneously in Inkscape 0.47 (~2 seconds).
This appears to be accurate... would you mind filing a bug report and assigning to Martin Sucha?
Unfortunately, there is no way we can release with this big of a performance hit for such a common task. So if it doesn't get fixed before Feature Freeze I will have to revert it.
Cheers, Josh
On 19/3/10 17:06, Joshua A. Andler wrote:
On Fri, 2010-03-19 at 16:48 +0100, ~suv wrote:
On 4/2/10 20:59, Joshua A. Andler wrote:
On Wed, 2010-02-03 at 19:10 +0100, Martin Sucha wrote:
Hi all,
I've implemented feature to automatically group objects when setting clip/mask and it now works for me. (Thanks Krzysztof Kosiński for help.)
Could you please review attached patch and commit it if you consider it good enough?
Reviewed and tested, looks good by me (and is something I will use). Committed in bzr rev 9050. Thank you!
Is it possible that this patch introduced a big performance loss when grouping a larger selection of objects? I just stumbled upon this when clipping ~1800 small objects (created with the spray tool) with a rectangle using the new feature to group the selection automatically before applying the clippath to the group: to my surprise this took very long (~45 seconds) whereas grouping the same selection happens almost instantaneously in Inkscape 0.47 (~2 seconds).
This appears to be accurate... would you mind filing a bug report and assigning to Martin Sucha?
Unfortunately, there is no way we can release with this big of a performance hit for such a common task. So if it doesn't get fixed before Feature Freeze I will have to revert it.
Bug #542004 https://bugs.launchpad.net/inkscape/+bug/542004 “performance regression when grouping a large selection set”
~suv
p.s. I had cancelled my previous message to the list because the attached file was too big. A sample test case is attached to the bug report.
On Friday 19 March 2010 17:38:15 ~suv wrote:
On 19/3/10 17:06, Joshua A. Andler wrote:
On Fri, 2010-03-19 at 16:48 +0100, ~suv wrote:
Is it possible that this patch introduced a big performance loss when grouping a larger selection of objects? I just stumbled upon this when clipping ~1800 small objects (created with the spray tool) with a rectangle using the new feature to group the selection automatically before applying the clippath to the group: to my surprise this took very long (~45 seconds) whereas grouping the same selection happens almost instantaneously in Inkscape 0.47 (~2 seconds).
This appears to be accurate... would you mind filing a bug report and assigning to Martin Sucha?
Unfortunately, there is no way we can release with this big of a performance hit for such a common task. So if it doesn't get fixed before Feature Freeze I will have to revert it.
Bug #542004 https://bugs.launchpad.net/inkscape/+bug/542004 “performance regression when grouping a large selection set”
Interesting. I changed the code so I could reuse grouping from clipping function, but the logic behind grouping should be the same. Looking at the patch, I suspect the call to selection->clear() should be before the whole grouping logic (it was before) and I accidentally put that after. Will check if this causes the regression.
Regards, Martin Sucha
On Friday 19 March 2010 18:40:26 Martin Sucha wrote:
On Friday 19 March 2010 17:38:15 ~suv wrote:
On 19/3/10 17:06, Joshua A. Andler wrote:
On Fri, 2010-03-19 at 16:48 +0100, ~suv wrote:
Is it possible that this patch introduced a big performance loss when grouping a larger selection of objects? I just stumbled upon this when clipping ~1800 small objects (created with the spray tool) with a rectangle using the new feature to group the selection automatically before applying the clippath to the group: to my surprise this took very long (~45 seconds) whereas grouping the same selection happens almost instantaneously in Inkscape 0.47 (~2 seconds).
This appears to be accurate... would you mind filing a bug report and assigning to Martin Sucha?
Unfortunately, there is no way we can release with this big of a performance hit for such a common task. So if it doesn't get fixed before Feature Freeze I will have to revert it.
Bug #542004 https://bugs.launchpad.net/inkscape/+bug/542004 “performance regression when grouping a large selection set”
Interesting. I changed the code so I could reuse grouping from clipping function, but the logic behind grouping should be the same. Looking at the patch, I suspect the call to selection->clear() should be before the whole grouping logic (it was before) and I accidentally put that after. Will check if this causes the regression.
Yes, this causes the regression. I fixed this for grouping and now I'll look at the clipping code as it needs to be adjusted as well. Will send a patch when done.
Regards, Martin Sucha
On Friday 19 March 2010 19:16:35 Martin Sucha wrote:
On Friday 19 March 2010 18:40:26 Martin Sucha wrote:
On Friday 19 March 2010 17:38:15 ~suv wrote:
Bug #542004 https://bugs.launchpad.net/inkscape/+bug/542004 “performance regression when grouping a large selection set”
Interesting. I changed the code so I could reuse grouping from clipping function, but the logic behind grouping should be the same. Looking at the patch, I suspect the call to selection->clear() should be before the whole grouping logic (it was before) and I accidentally put that after. Will check if this causes the regression.
Yes, this causes the regression. I fixed this for grouping and now I'll look at the clipping code as it needs to be adjusted as well. Will send a patch when done.
I've attached the patch to the bug report (#542004). Could you please try that? I tried to test all possible code paths, however, I don't know how to apply a clip on layer so please test it and/or tell me how can I do that.
This patch moves selection into list so hopefully I've got that right. I don't know if I managed to maintain the selection order (for clipping and removing clip), though; how to test the selection order and whether it is an issue.
Additionally, I also fixed a performance problem (related to selection) that is present in 0.47 - it shows when you ungroup a group and have selected a lot of non-group objects as well.
Regards, Martin Sucha
On Mar 19, 2010, at 4:09 PM, Martin Sucha wrote:
Additionally, I also fixed a performance problem (related to selection) that is present in 0.47 - it shows when you ungroup a group and have selected a lot of non-group objects as well.
Is there any chance we can bounce such things through some automated test?
Things are always tricky when it comes to performance, but perhaps an end-to-end via command line invocation could be worked up to exercise this feature.
Thanks.
2010/3/20 Jon Cruz <jon@...18...>:
Is there any chance we can bounce such things through some automated test?
Things are always tricky when it comes to performance, but perhaps an end-to-end via command line invocation could be worked up to exercise this feature.
Grouping, assigning a clipping path and a few other operations (e.g. pasting a document into another one) should be a method in the SP layer. This way it would be easier to run performance tests on it, and there would be only one implementation to check for correctness.
Regards, Krzysztof
On Mar 19, 2010, at 6:45 PM, Krzysztof Kosiński wrote:
2010/3/20 Jon Cruz <jon@...18...>:
Is there any chance we can bounce such things through some automated test?
Things are always tricky when it comes to performance, but perhaps an end-to-end via command line invocation could be worked up to exercise this feature.
Grouping, assigning a clipping path and a few other operations (e.g. pasting a document into another one) should be a method in the SP layer. This way it would be easier to run performance tests on it, and there would be only one implementation to check for correctness.
Well... it might help for correctness, but not really for performance test. Micro benchmarks are not nearly as accurate or useful as full end-to-end testing.
It's that end-to-end performance that we need to look at first. Once we get some "big picture" measurements going, then it will be more helpful to check specific subsets, operations in isolation, etc.
On Saturday 20 March 2010 01:49:56 Jon Cruz wrote:
On Mar 19, 2010, at 4:09 PM, Martin Sucha wrote:
Additionally, I also fixed a performance problem (related to selection) that is present in 0.47 - it shows when you ungroup a group and have selected a lot of non-group objects as well.
Is there any chance we can bounce such things through some automated test?
Things are always tricky when it comes to performance, but perhaps an end-to-end via command line invocation could be worked up to exercise this feature.
Maybe D-Bus stuff could be used to test those?
Regards, Martin Sucha
participants (5)
-
Jon Cruz -
Joshua A. Andler -
Krzysztof Kosiński -
Martin Sucha -
~suv