fulltests/Wave.svg's red nose and white torso get eaten

[Louis Simard]

After Scouring, the file fulltests/Wave.svg in Bazaar for Scour renders incorrectly in every SVG library I can find.

Wave.svg is better known as Duke, Sun's mascot for Java, and it has a red nose and a white torso.

After optimisation in Scour, the nose is not rendered, although it's in the output file. The same thing happens with the torso.

Attached are the Scoured file and its expected and actual renders.

[Louis Simard (louis-simard-deactivatedaccount)]

It looks to be the <switch>. There was a <g> containing all of those paths in the original file, and now I guess the <switch> is selecting only the first path.

Groups shouldn't be collapsed if the immediate parent is a <switch>. I'll code something for that.

[codedread (codedread)]

This file worked perfectly fine in r171, so it's definitely something you did :)

Tip: Make smaller changes one-at-a-time and check them once you're sure they work. Do all the unit tests still work?

[Louis Simard (louis-simard-deactivatedaccount)]

The file couldn't have worked fine in r171, because Scour did group collapsing before I changed anything. I'll check it again.

The unit tests still work, all 153 of them.

[codedread (codedread)]

Actually I take it back - the latest scour file scours it fine. I'm attaching the SVG. Can you attach the actual scoured SVG that has the problem (PNGs are useless)

[codedread (codedread)]

My bad again - it seems you did attach the wrong SVG. I'll try to compare the two.

[Louis Simard (louis-simard-deactivatedaccount)]

Forgot to note the command-line I used to Scour this file in r172+:

./scour.py --shorten-ids --enable-id-stripping --remove-metadata --enable-comment-stripping --strip-xml-prolog -i fulltests/Wave.svg -o fulltests/Wave.opt.svg

[Louis Simard (louis-simard-deactivatedaccount)]

The main difference between your file and mine is that there's a <g id="Layer_1"> in yours.

Then the real problem here is that I'm using --enable-id-stripping, and so the <g> without an ID becomes useless and is collapsed.

Should I add a bit of code that detects <switch> used as the immediate parent of a <g> and avoids collapsing automatically in that case?

[codedread (codedread)]

Ah, I see! :)

I agree with your logic (do not remove a <g> if it is the immediate child of a <switch>), that will solve this rendering problem.

But I would also argue that removing the group actually bloats the markup much more than removing the <g> in the first place - look at all the imposed cilp-rule and fill-rule attributes on the 162 <path> elements. This is probably my fault (earlier logic I added and never tested out the options against real-world files).

I think we need to look at the logic behind reducing groups - if it imposes more XML attributes than it solves by eliminating a layer of nesting, I'd say keep the group.

Jeff

[Louis Simard (louis-simard-deactivatedaccount)]

Wow. I didn't notice the attributes being propagated, but your file being smaller than mine by 6 KB is an indication of failure to optimise indeed.

In either case, 'g' *must* check that its parent is not a 'switch', so I'll probably commit a quick fix for this first before attacking the rest.

<switch> <g> <g ATTRIBUTES> should at least remove the outer 'g', but preserve the inner g's attributes as you describe.

However, <switch> <g> <g id="UNUSED"> should remove the outer 'g', but *must* preserve the inner 'g' on the grounds that it's the only 'g' keeping the 'switch' correct.

[codedread (codedread)]

Louis, right.

Please construct a couple unit tests for these cases so that we can ensure future coverage.

Thanks,
Jeff

[Louis Simard (louis-simard-deactivatedaccount)]

Since this bug was about the rendering specifically, I'll mark this as Fix Committed.

I added 2 unit tests for <switch><g>, one to make sure it doesn't get collapsed and another to make sure that it won't even get collapsed if --enable-id-stripping is set and the <g>'s ID name is unused.

Duke now renders correctly, and in fact the fix restored the blue roof in fulltests/apartment.svg as well. The Duke file is smaller than yours, meaning that the 162 clip-rules and fill-rules were put into the <g>. I don't know if it's a fluke related to the file, or if it's the result of moveCommonAttributesToParentGroup.