-
Notifications
You must be signed in to change notification settings - Fork 685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the view bounds as the intrinsic size of an SVG in SvgDecoder. #688
Conversation
@@ -1,7 +1,7 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"> | |||
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" width="200px" | |||
height="200px" viewBox="332.5 200 140 140" xml:space="preserve"> | |||
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" width="1000px" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this so the test cases fail if it uses the width
/height
for the aspect ratio.
class SvgDecoder(private val context: Context) : Decoder { | ||
class SvgDecoder @JvmOverloads constructor( | ||
private val context: Context, | ||
private val useViewBoundsAsIntrinsicSize: Boolean = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay that this changes behavior for those who are using this currently? Maybe it should default to false until a 1.2 or a 2.0 so that folks don't update their library and end up with unexpected behavior? Otherwise, I'd just make sure its communicated clearly in the CHANGELOG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jawnnypoo Good point. Will call this out at the top of the release notes.
I'm definitely for only enabling this in 1.2
(which will be the next release). 2.0
is still a ways away so I'd want to introduce the behaviour change before then. Also I'm hoping this change will be invisible for most users as for most SVGs width
/height
already has the same aspect ratio as viewBox
. This change mostly improves the handling for weird SVGs that Coil wouldn't render properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! Thanks for clarifying 👍
Could you clarify why should the intrinsic size of an SVG be its view box size (if present)? You mentioned in changelog that this way it would "correctly follow the SVG specification", but I couldn't find relevant specification in W3C SVG Specification. Meanwhile, I tried to create an SVG with width
|
@zhanghai Sorry, the title of this PR isn't accurate. This PR changes the logic so the view box is used to compute the aspect ratio to fit into the target size. For example, if we have an SVG that is 100x200, but the view box is only 100x100, the image actually only has a 1x1 aspect ratio (everything outside of the view box isn't rendered). If the target size is |
I see, thanks for the clarification about using view box for fitting the target size. However, it still seems to me the original behavior is correct.
This doesn't sound correct according to my understanding of SVG.
|
Currently we use an SVG's
width
/height
to determine its intrinsic size and its aspect ratio. However, we should actually be using itsviewBox
since that's the actual size of the rendered content. e.g. we could have an SVG that's 500x500, but a view box that's only the center 100x200 pixels. In that case the aspect ratio of the rendered content is 1x2 and not 1x1.Also adds a
useViewBoundsAsIntrinsicSize
constructor param to opt out of the changes if it causes issues.