Implement wxSVGFileDC::DoSetClippingRegion / DestroyClippingRegion

[steveb]

wxSVGFileDC (v2.9.3) does not implement SetClippingRegion / DestroyClippingRegion.

I've derived classes from wxSVGFileDC and wxSVGFileDCImpl that implement this functionality in a way that's suitable for use in an example application that I'm working on. I'm not an SVG expert (far from it) so am not submitting a suggested patch for the wxWidgets trunk, but perhaps this could be a starter-for-ten for somebody who understands it better to make an appropriate update to wxSVGFileDC and wxSVGFileDCImpl.

The implementation required one minor change to dcsvg.h to change the 'void write( const wxString &s )' method on wxSVGFileDCImpl to protected (was private) so that the SVG file stream could be written to in the derived class. This is perhaps a useful update to make in general, since it will allow future derived classes to access the SVG file stream.

The implementation is fairly self-explanatory (and short). Only the following is worth mentioning: the implementation of DoSetClippingRegion in my derived class (wxSVGFileDCWithClippingImpl) simply constructs a <clipPath> SVG element and writes it to the SVG stream. After this it begins an SVG group <g style="clip-path..."> element for subsequent drawing instructions to be entered in. I found that my subsequent instructions triggered a call to NewGraphics() (on wxSVGFileDCImpl), which as its first action writes a </g> to the SVG stream and hence immediately closes the clipping group that was started with my DoSetClippingRegion method.

To solve this in my application it is sufficient to write a </g> first in my DoSetClippingRegion to close the previous group, before beginning the new group. I also then have to write an additional superfluous <g> to the stream after the <g style="clip-path..."> element, which is then 'cancelled out' by the </g> that is written by NewGraphics(). The resulting SVG looks like this:

</g> <!-- SJB end previous group -->
<defs>

<clipPath id="clip1">

<rect id="cliprect1" x="80" y="30" width="358" height="129" style="stroke: gray; fill: none;"/>

</clipPath>

</defs>
<g style="clip-path: url(#clip1);"> <!-- SJB group for clipped objects -->
<g> <!-- SJB to counteract end group that is added below -->
</g>

(The final </g> here is the one written by NewGraphics().)

[steveb]

Derived classes (from wxSVGFileDC/wxSVGFileDCImpl) implementing SetClippingRegion/DestroyClippingRegion

[vadz]

I'm not an SVG expert neither unfortunately but integrating your code into wxWidgets should be pretty simple: just remove the (trivial) bodies of the functions you overrode from include/wx/dcsvg.h and define them in src/common/dcsvg.cpp. Then please make a patch with your changes so that it could be applied easily.

Ideal would be to also have some sort of a test for this functionality, e.g. a demonstration of using clipping in samples/svg/svgtest.cpp.

If you can do it, it would be very welcome -- thanks in advance!

[steveb]

Thanks for your feedback.

I'll work on it a little more to try to make my implementation a little more robust - I'm worried that the </g> and <g> that I slip in to make the code work for my application might not work in general if calls to NewGraphics don't immediately follow DoSetClippingRegion.

Once done I'll make a simple example/test.

[steveb]

Hi I've created a proposed patch for an update to wxSVGFileDC to allow it to implement clipping regions. The patch is a diff against wxWidgets-2.9.3.

The updates were a bit more detailed than my hack outlined in the earlier correspondence. It became apparent that a facility for managing the creation of SVG group (<g>) elements was needed. The reason for this is that in wxDC, some instructions are like state changes (change of pen, brush, font, etc.) whilst others are like start_feature ... end_feature switches (SetClippingRegion ... DestroyClippingRegion).

The nested XML structure of SVG is not really best-suited for drawing with a combination of state changes and start_feature ... end_feature switches (although note my earlier admission to not being an SVG expert - somebody may know a better way to do this). For example, suppose we want to:

set pen pA
(draw stuff 1)
start clipping cA
set pen pB
(draw stuff 2)
destroy clipping
(draw stuff 3)

If the SVG groups <g> representing changes of pen are nested within the clipping regions then pen pA would be used in 'draw stuff 3' above, because the 'set pen pB' group will have to be closed before the group representing the clipping region cA is closed. However the corresponding set of instructions on a standard wxDC would cause pen pB to be used in the 'draw stuff 3' instructions.

To overcome this, I've implemented some SVG group management in wxSVGFileDC so that state change groups are closed before every start_feature group opener. Immediately after, the current state is the re-applied, so that the state persists until the end_feature group close (unless any new states are applied). Immediately after the end_feature group close the current state is applied again. This results in some repetition in the SVG, but is the only way that I could see of ensuring that the current state (from the standard wxDC) is applied consistently in the SVG nesting.

With this approach, the above set of instructions become:

set pen pA
(draw stuff 1)
(close pen pA) - automatic: to close group for current state
start clipping cA
set pen pA - automatic: to re-apply the current state
(close pen pA) - automatic: as first step of set pen pB that comes next
set pen pB
(draw stuff 2)
(close pen pB) - automatic: as first step of destroy (close) clipping that comes next
destroy clipping
set pen pB - automatic: to re-apply the current state
(draw stuff 3)
...
</g>...</g></svg> - automatic: to ensure valid XML structure.

Note that there are no extra instructions needed from the client above - all the new instructions to ensure correct states and valid XML are automatic.

Note also that the situation regarding DestroyClippingRegion is slightly more complicated by the fact that SetClippingRegion can be called repeatedly, with subsequent calls causing the intersection of the new region with the existing clipping region being used for subsequent clipping. However DestroyClippingRegion is only called once to kill al clipping (i.e. there is not a clipping stack that needs to be undone). However in the SVG, the intersection of clipping regions by subsequent calls to SetClippingRegion is implemented by nesting <g> clipping groups (causes SVG to calculate the intersection). So the single DestroyClippingRegion has to close multiple <g> clipping groups. Again this is all handled automatically and the current state is maintained.

Apologies for the essay here, but I thought it useful to explain why the patch is perhaps a little 'bigger' than you might have been expecting. Since some of these concerns are perhaps non-obvious (until you think about them), I've also repeated some of this explanation in comments in the header and cpp. This may be a too wordy for your coding standards - please crop as you see fit.

I've updated svgtest.cpp with a new test for clipping (implemented in the new 7th tab in the window) and have also updated the interface documentation. I'll upload an image of the saved SVG from the test so you can see what it should look like in case there are any problems.

This is the first time I've tried to submit a patch, so apologies if I've missed anything out. Do let me know if you want anything changed.

[vadz]

Steve, thanks for the explanations, they make sense to me and very helpful. Unfortunately I don't have time to really review the entire patch now and won't be able to do it before August at earliest.

So it would be great if others could test it and leave their impressions here.

From a superficial look at the patch I see some things which would need to be changed before applying it:

1. "wx/vector.h" and wxVector<> should be used instead of <vector> and std::vector<>. They have (almost) the same interface so it shouldn't be a problem to change this.
2. Some indentation in the patch is strange, I suspect it contains hard TAB characters, please avoid their use in wx sources, use 4 spaces for indentation instead.
3. More generally, please try to conform to our  coding standards, e.g. put spaces around == and stuff like that.

But, again, the most important thing is to actually review and/or test the patch to ensure that it does work correctly and it would be great if somebody familiar with this code could do this (Chris?).

[steveb]

Thanks for the feedback. No problem to switch to wxVector<> and the other coding standards. I'll do that and resubmit.

(Also sorry about the tabs. I thought I'd removed all of these, but obviously my sed skills are worse than I'd thought...)

As you say, it would be helpful to get feedback from somebody who's familiar with the source. I'd welcome any comments.

[steveb]

I've uploaded an updated patch (v2) addressing the vector / coding standard issues. wxVector was a straight swap as you suspected. No functional changes.

[vadz]

Could someone using SVG please test this? I really don't have time to study this code in details and would appreciate if it could be tested by at least one other person before being committed.

[vadz]

I've finally looked at this code in more details and I hate to say it but I think it's just too complicated to be committed in the current state. Maybe you can explain to me what am I missing but I just don't understand what do we need the group categories and group types when there seems to be one-to-one correspondence between the two -- so leaving just one of them would be the first simplification I'd make.

The second one would concern EndGroup(): it seems to be only ever called for GroupType_ClippingRegion. Either this is a bug or we don't need half of its code. I initially thought it was a bug but the comment in it seems to say that this intentional. If so, I really disagree with this, it's a very bad idea to write code that is not used at all currently, this just makes things more confusing, without speaking of the wasted effort required to read/understand/maintain it.

Moreover, the only call to this function I see is with endAllGroupsOfType == true -- so why do we even have this parameter anyhow? I'd just have a simple (at least, simpler) EndAllOnOffGroups() method instead of the current one.

I even thought to simplify the code myself like this to finally do something about this patch but then I had another, even more fundamental question: how can this work when there doesn't seem to be any relationship between the vectors containing two kinds of groups? It seems to me that there should be some or how can we avoid reapplying too many pen/brush changes after closing a group? For this matter, why do we even need to remember any but the very last pen/brush change? So I'm afraid I really don't understand how is this supposed to work...

The basic idea, i.e. what is explained in the comments above and in the code in the patch looks correct to me but I don't understand how does it map to the code itself. Could you please explain?

[steveb]

Hi,

many thanks for finding the time to look at the patch - it's appreciated.

I probably should have explained better when I submitted that patch. My thinking was that in future there might be further additions to wxSVGFileDC that take advantage of some of the other SVG features. (See earlier post though - I'm not an SVG expert so the following might be wrong!) From their xml structure, SVG commands can be nested and so pretty much any set of commands can be made to operate in a stack sense. So clipping regions could be grown and shrunk in push/pop fashion, rather than the way wxDC supports them. If there are similar constructs that could be formed in this way in future iterations of wxDC (e.g. operations on unions of regions, or any functionality that is temporarily switched on and then off) then my hope was that the code that I'd provided could be applied immediately to those new features without much development needed. That's why the enumerations with the 1-1 mapping that you note are there - my hope was that in future they would grow.

As you point out though, it's perhaps not best to pre-empt problems, since that results in code lying around that's largely unused, poorly understood and not thoroughly tested until something that needs it comes along. So I've produced a v3 patch. This one just implements the clipping in a more direct fashion that's largely a tidy-up of my original hack. It should be much easier to understand.

Sorry - in future patches I'll aim for a more direct fix rather than trying to predict the future..!

Thanks again.

[vadz]

Thanks, this patch indeed looks much more reasonable. I've done some small changes to this code in the meanwhile so it won't apply cleanly right now but I'll try to merge it when I have time next.

Thanks again!

[VZ]

(In [72760]) No changes, just remove unused variable from wxSVGFileDCImpl.

sWarn was never used, drop it.

See #14462.

[VZ]

(In [72761]) No real changes, just avoid unnecessary string operations in wxSVGFileDCImpl.

Simply write string together instead of concatenating them during run-time.

See #14462.

[VZ]

(In [72762]) Implement clipping in wxSVGFileDC.

Support setting the clipping region and add update the documentation and the
sample accordingly.

Closes #14462.