rafaeltonholo / svg-to-compose

A command-line tool for convert SVG to Android Jetpack Compose Icons.
MIT License
61 stars 2 forks source link

Consider when the fill-rule is even-odd in the generated code #25

Closed StylianosGakis closed 4 months ago

StylianosGakis commented 5 months ago

For an svg like this

<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">
<g id="Icon/24/Attention-Filled">
<path id="Subtract" fill-rule="evenodd" clip-rule="evenodd" d="M21.5 12C21.5 17.2467 17.2467 21.5 12 21.5C6.75329 21.5 2.5 17.2467 2.5 12C2.5 6.75329 6.75329 2.5 12 2.5C17.2467 2.5 21.5 6.75329 21.5 12ZM12 7.25C12.4142 7.25 12.75 7.58579 12.75 8V13C12.75 13.4142 12.4142 13.75 12 13.75C11.5858 13.75 11.25 13.4142 11.25 13V8C11.25 7.58579 11.5858 7.25 12 7.25ZM12 17C12.5523 17 13 16.5523 13 16C13 15.4477 12.5523 15 12 15C11.4477 15 11 15.4477 11 16C11 16.5523 11.4477 17 12 17Z" fill="#121212"/>
</g>
</svg>

Which shows a circle with a exclamation mark in the middle, and should be rendered like this

image

When using this tool (with optimization = false) to generate a composable I get this

val AttentionFilled: ImageVector
    get() {
        val current = _attentionFilled
        if (current != null) return current

        return ImageVector.Builder(
            name = "AttentionFilled",
            defaultWidth = 24.0.dp,
            defaultHeight = 24.0.dp,
            viewportWidth = 24.0f,
            viewportHeight = 24.0f,
        ).apply {
            group {
                // M21.5 12 C21.5 17.2467 17.2467 21.5 12 21.5 C6.75329 21.5 2.5 17.2467 2.5 12 C2.5 6.75329 6.75329 2.5 12 2.5 C17.2467 2.5 21.5 6.75329 21.5 12Z M12 7.25 C12.4142 7.25 12.75 7.58579 12.75 8 V13 C12.75 13.4142 12.4142 13.75 12 13.75 C11.5858 13.75 11.25 13.4142 11.25 13 V8 C11.25 7.58579 11.5858 7.25 12 7.25Z M12 17 C12.5523 17 13 16.5523 13 16 C13 15.4477 12.5523 15 12 15 C11.4477 15 11 15.4477 11 16 C11 16.5523 11.4477 17 12 17Z
                path(
                  fill = SolidColor(Color(0xFF121212)),
                ) {
                    // M 21.5 12
                    moveTo(x = 21.5f, y = 12.0f)
                    // C 21.5 17.2467 17.2467 21.5 12 21.5
                    curveTo(
                        x1 = 21.5f,
                        y1 = 17.2467f,
                        x2 = 17.2467f,
                        y2 = 21.5f,
                        x3 = 12.0f,
                        y3 = 21.5f,
                    )
                    // C 6.75329 21.5 2.5 17.2467 2.5 12
                    curveTo(
                        x1 = 6.75329f,
                        y1 = 21.5f,
                        x2 = 2.5f,
                        y2 = 17.2467f,
                        x3 = 2.5f,
                        y3 = 12.0f,
                    )
                    // C 2.5 6.75329 6.75329 2.5 12 2.5
                    curveTo(
                        x1 = 2.5f,
                        y1 = 6.75329f,
                        x2 = 6.75329f,
                        y2 = 2.5f,
                        x3 = 12.0f,
                        y3 = 2.5f,
                    )
                    // C 17.2467 2.5 21.5 6.75329 21.5 12z
                    curveTo(
                        x1 = 17.2467f,
                        y1 = 2.5f,
                        x2 = 21.5f,
                        y2 = 6.75329f,
                        x3 = 21.5f,
                        y3 = 12.0f,
                    )
                    close()
                    // M 12 7.25
                    moveTo(x = 12.0f, y = 7.25f)
                    // C 12.4142 7.25 12.75 7.58579 12.75 8
                    curveTo(
                        x1 = 12.4142f,
                        y1 = 7.25f,
                        x2 = 12.75f,
                        y2 = 7.58579f,
                        x3 = 12.75f,
                        y3 = 8.0f,
                    )
                    // V 13
                    verticalLineTo(y = 13.0f)
                    // C 12.75 13.4142 12.4142 13.75 12 13.75
                    curveTo(
                        x1 = 12.75f,
                        y1 = 13.4142f,
                        x2 = 12.4142f,
                        y2 = 13.75f,
                        x3 = 12.0f,
                        y3 = 13.75f,
                    )
                    // C 11.5858 13.75 11.25 13.4142 11.25 13
                    curveTo(
                        x1 = 11.5858f,
                        y1 = 13.75f,
                        x2 = 11.25f,
                        y2 = 13.4142f,
                        x3 = 11.25f,
                        y3 = 13.0f,
                    )
                    // V 8
                    verticalLineTo(y = 8.0f)
                    // C 11.25 7.58579 11.5858 7.25 12 7.25z
                    curveTo(
                        x1 = 11.25f,
                        y1 = 7.58579f,
                        x2 = 11.5858f,
                        y2 = 7.25f,
                        x3 = 12.0f,
                        y3 = 7.25f,
                    )
                    close()
                    // M 12 17
                    moveTo(x = 12.0f, y = 17.0f)
                    // C 12.5523 17 13 16.5523 13 16
                    curveTo(
                        x1 = 12.5523f,
                        y1 = 17.0f,
                        x2 = 13.0f,
                        y2 = 16.5523f,
                        x3 = 13.0f,
                        y3 = 16.0f,
                    )
                    // C 13 15.4477 12.5523 15 12 15
                    curveTo(
                        x1 = 13.0f,
                        y1 = 15.4477f,
                        x2 = 12.5523f,
                        y2 = 15.0f,
                        x3 = 12.0f,
                        y3 = 15.0f,
                    )
                    // C 11.4477 15 11 15.4477 11 16
                    curveTo(
                        x1 = 11.4477f,
                        y1 = 15.0f,
                        x2 = 11.0f,
                        y2 = 15.4477f,
                        x3 = 11.0f,
                        y3 = 16.0f,
                    )
                    // C 11 16.5523 11.4477 17 12 17z
                    curveTo(
                        x1 = 11.0f,
                        y1 = 16.5523f,
                        x2 = 11.4477f,
                        y2 = 17.0f,
                        x3 = 12.0f,
                        y3 = 17.0f,
                    )
                    close()
                }
            }
        }.build().also { _attentionFilled = it }
    }

However this renders this:

image

Going in and adding this line pathFillType = PathFillType.EvenOdd, to the path() makes this work again, but I had to do it manually here.

Note that when I use the optimized flag then it is not fixed even with changing this pathFillType = PathFillType.EvenOdd, which I suppose happens as the optimization does not take this into consideration so it breaks the svg in unexpected ways.

I am not sure exactly which part it is that is failing here. Is the fill type not read from the svg file, or is our svg file somehow not compatible with what this tool can do, or what else could I do to encounter the correct behavior correctly?

Perhaps I could just run all the icons that it imports wrong though the non-optimized script and then go in an manually try to match the fill types as a workaround for now, but that can be quite some work.

StylianosGakis commented 5 months ago

So for us, I just run all of our icons again, all of them with the unoptimized script and then just did pathFillType = PathFillType.EvenOdd, to all of them and now I seem to be getting the correct output from all of them. Thanks so so much for this tool btw, this is amazing that it can do all this. It would still be interesting to understand if I can use it in a different way so that I can still get the optimized versions and still be correct at the same time

rafaeltonholo commented 5 months ago

Hello @StylianosGakis,

Thank you for bringing this issue to my attention and for providing the SVG file for testing, it really helps! I'm sorry to hear that you are experiencing this problem.

The issue you are encountering is actually a bug. It is occurring because the fill-rule attribute in SVG is lowercase, while its corresponding attribute in AVG is camel case. When I added support for this attribute, I forgot to verify that they have the same values.

When I was working on another feature, I could reproduce this issue, so I created a fix addressing this issue in its branch (https://github.com/rafaeltonholo/svg-to-compose/commit/5204596b76434e38148db2dc6f23dbc5b4490baf). However, I haven't released the fix yet because I didn't have time to finalize the content of that feature.

Since you are being affected by this bug, I am considering doing a cherrypick in that branch and releasing a bugfix version. This will allow you to generate the icons properly.

Thank you for your feedback about the CLI. I'm glad to hear that it is helping you with your needs.

StylianosGakis commented 5 months ago

That sounds great Rafael! Thanks a lot for the quick response too. I will be happy to give the new release a try as soon as it's ready to confirm (or not) that this fix works! I'll try to follow the releases here to make sure to give it a try as soon as that happens and I find the time.

StylianosGakis commented 5 months ago

Come to think of it, can I just check out that branch myself and run the script locally with those code changes so that I can verify it myself before having to make a release on your end?

rafaeltonholo commented 5 months ago

Come to think of it, can I just check out that branch myself and run the script locally with those code changes so that I can verify it myself before having to make a release on your end?

I wouldn't use the feat/handle-gradient-and-item-tags, at least not for now, since I don't remember if the head commit is properly working as I still have some local changes.

I have created a PR #29 by doing a cherry pick only on the changes that address that bug. I did some tests with the samples I have in this repository and looks like everything is fine with the changes.

I also tested the provided icon and looks like it is working with the changes.

If you want to test it on your end, I would recommend using the bugfix/compose-types-lowercase branch instead.

StylianosGakis commented 5 months ago

After checking out that branch, will it just work for me to use the s2c script, or does that need to be configured in some way to use the local code of that branch?

rafaeltonholo commented 5 months ago

After checking out that branch, will it just work for me to use the s2c script, or does that need to be configured in some way to use the local code of that branch?

If you have cloned the repository, you can checkout the bugfix branch and run the s2c script with the same command you were previously using. However, this time you need to add a --upgrade flag before the path. Here's an example command that you can use:

./s2c -o <output-path> \
      -p <package> \
      --theme <theme> \
      --upgrade \
      <icon-paths>

Adding the --upgrade flag will remove the binaries and build them again, allowing you to use a binary with the bug fix applied.

If you have just downloaded the s2c script, please note that the s2c script alone only looks for the latest Release. Hence, you will need to wait until I merge the PR, so it will generate a Release with new binaries with the bugfix. If that is the case, let me know, and I will merge it right away so you can test it.