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

Bug #594930 reported by Louis Simard on 2010-06-16
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Scour
Medium
Unassigned

Bug Description

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.

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) wrote :

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?

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.

Changed in scour:
importance: Undecided → Medium
status: New → Triaged
codedread (codedread) wrote :

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) wrote :

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

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

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) wrote :

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

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) wrote :

Louis, right.

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

Thanks,
Jeff

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.

Changed in scour:
status: Triaged → Fix Committed
codedread (codedread) on 2010-07-12
Changed in scour:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers