A more sophisticated SVG implementation.

[pekim]

This is very much a proof of concept, and not in a fit state to be merged yet.

I want to be able to use some SVGs that are more complicated than what unison currently supports. This PR shows a different approach to drawing SVGs in unison.

Instead of parsing the XML in unison, the lafriks/go-svg library is used. It supports quite a lot more than unison currently does. For example it will apply translate transformations in <g>s to the nested paths. Shapes such as <ellipse> and <circle> as transformed in to paths. Fill and stroke attributes in <g>s are inherited by child paths. And more.

The implementation that I've put together implements path stroking as well as filling.

The crude example in the example_svg directory draws a circled chevron svg with both the SVG and the new SVG2 types, and they appear to be identical. It also draws a more complex gopher svg, and the classic tiger svg.

~I originally created the SVG2 and Path2 types intending them to be temporary, while proving the approach. My intention was that once it was all working I'd simply replace the existing SVG and Path with the new implementations. However the new implementation can't exactly implement the existing api. The paint *Paint argument to DrawableSVG#DrawInRect cannot be honored in the new implementation, as the Paints that are used for filling and stroking paths are created from the relevant attributes in the SVG. So if these changes were to be accepted they would probably have to remain as new types. Although they'd need better names than SVG2 and Path2.~ EDIT: Later changes have made this paragraph redundant.

It all looks reasonably promising to me. However before I put much too more effort in, I'd like to know if there is any interest in any of this? Of course I'd tidy up the scrappy commits (probably squashing to a single commit), and accommodate any suggestions you have.

[pekim]

The paint *Paint argument to DrawableSVG#DrawInRect cannot be honored in the new implementation, as the Paints that are used for filling and stroking paths are created from the relevant attributes in the SVG. So if these changes were to be accepted they would probably have to remain as new types.

I've pushed 4db534c that I think provides backwards compatibility for the paint argument. So replacement of the SVG type with the new version may yet be possible.

Path would still need to be kept, as it's used in other places, such as Canvas. But Path2 is essentially private to the new SVG implement, and could be not exported from the package.

I'm not familiar enough with all of the inner workings of unison, so I expect that I've overlooked a few more things.

[pekim]

I've combined Path2 in to Path, and SVG2 in to SVG. So I've managed to make it all backwards compatible. At least I think so.

I think that this is all in a moderately reasonable state now. It's backwards compatible, I can't find anything in the big example application that looks wrong, and more complex SVGs can be rendered.

[richardwilkes]

I am indeed interested in something along these lines. It has been on my list for a while to implement the necessary bits, but just haven't found the time (nor the actual need -- none of my other projects actually need it right now).

I've only glanced at what you've done so far, since it seems you're still actively changing things. Let me know when you're done and I'll give it an actual review.

Some things of concern in my quick glance:

• It appears you've removed some methods from the Path object. All of those methods are used in some of my other projects even if they aren't used in unison itself, so removing this isn't acceptable. Some of them may not be obvious what they are intended for -- I'm happy to explain any that are a mystery.
• I'm a bit concerned that the changes now include several other drawing packages (all are decent, and I've looked at them in the past, but they are all fundamentally slow due to being CPU-based, which made them a non-starter for me). Given the way go packaging works, this may not be a real issue... but it is something I'd want to examine. It might be better to fork/extract the parsing logic into a separate repo somewhere and remove the rasterizers to guarantee they don't get pulled in.

Anyway, thanks for looking at this. I think it will be quite useful.

[pekim]

It appears you've removed some methods from the Path object.

I think that you meant SVG methods not Path methods. Sorry about that, I have re-instated the methods.

I'm a bit concerned that the changes now include several other drawing packages (all are decent, and I've looked at them in the past, but they are all fundamentally slow due to being CPU-based, which made them a non-starter for me). Given the way go packaging works, this may not be a real issue... but it is something I'd want to examine. It might be better to fork/extract the parsing logic into a separate repo somewhere and remove the rasterizers to guarantee they don't get pulled in.

There is one new direct dependency, github.com/lafriks/go-svg. It is only used to parse an SVG. None of its rendering capabilities are used, and none of its rendering packages are imported by its parsing code. After parsing an SVG Paths are created just like unison's current implementation. The skia paths are only drawn to a canvas when DrawableSvg#DrawInRect is called.

I have looked at the symbols in a built unison app binary (using nm). While there are the expected occurences of lots of symbols starting github.com/lafriks/go-svg, there are none with github.com/lafriks/go-svg/renderer. So I think that it's a safe bet that none of its rasterising code is pulled in. Therefore I don't believe that it's necessary to fork the library.

[pekim]

I think that I've finished making changes. So it's ready for a review.

A few things worth drawing attention to.

• Previously all paths were combined in to a single path during SVG construction. This is no longer done because in more complex SVGs different paths may have different filling and stroking attributes. So the SVG type has a slice of *Path, and each of the paths are drawn when the svg is drawn. A convenience SVG#CombinedPath method has been added, that (lazily) combines all paths.
• For now only solid colours are supported for fills and strokes. It probably wouldn't be too difficult to add support for linear and radial gradients in the future, but I don't want to overcomplicate this PR.
• Stroke dashing is another potential future enhancement.
• The example app in the example_svg directory can possibly be removed before merging. I created it for rendering various SVGs and verifying their appearance during development and review.

[pekim]

As I noted in at least one of them, I'm willing to actually do these things myself if you'd rather not, but I also don't want to take away the potential satisfaction you might get by doing it yourself

I'm happy to try to see it through.

I think that I've addressed all of the issues that you've raised. But of course you may want to see some of them addressed a little differently.

[richardwilkes]

So for the actual drawing in DrawInRect()... I was thinking it should behave more like you get on a web page. i.e. most svg's don't specify a color at all, or specify "currentColor", which allows the web page to colorize those parts of the image that don't have an explicit color assigned. Ideally, the paint parameter would be used for those cases, but it wouldn't override existing explicitly set colors. I haven't looked close enough at the lafriks/go-svg implementation to know if you can detect these cases correctly... but I would hope that's possible.

[richardwilkes]

Also, it looks like you still have a fair amount of code in the path.go file. I'm advocating that no changes are needed in that file (i.e. revert it to its original state) and everything that is being done there can be moved into the svg.go file. They are in the same package, so there are no access restrictions, and I'd like to keep all of the svg-specific stuff out of that file.

[pekim]

The remaining changes needed seem to be related to where some of the code belongs, and what the exposed api looks like. And you know much better than me how you'd like that.

So if you're okay with it, I'd like to take you up on your earlier offer of "I'm willing to actually do these things myself"?

(I've enabled Allow edits by maintainers for the PR.)

[richardwilkes]

OK, I'm going to go ahead and merge this as-is, then follow up with the additional changes. Thanks for all the work, @pekim !