Skip to content
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

fix: Do not render SVGs bigger than requested when pixelRatio > 1 and no match in _imageCache #2795

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

l1553k
Copy link
Contributor

@l1553k l1553k commented Oct 4, 2023

Description

When rendering the SVG for the first time, on devices with pixelRatio > 1, the image is rendered bigger. Only subsequent renders that utilize _imageCache are rendering the SVG properly.

Checklist

  • I have followed the [Contributor Guide] when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • No, this PR is not a breaking change.

Tested on screens with pixelRatio=1 (LG, 2560x1080) and =2 (MacBook Pro M1)

@l1553k
Copy link
Contributor Author

l1553k commented Oct 4, 2023

Screen.Recording.2023-10-04.at.14.10.29.mov

Dragging a mouse with left button pressed allows resizing that circle. During the movement, SVG is repeatedly rendered with different sizes, so the rendering is done by the "else" branch of Svg.render

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, lgtm!

@l1553k
Copy link
Contributor Author

l1553k commented Oct 4, 2023

@spydon I checked rendering the same SVG with many different sizes (after all, that is how I found this bug) and my "svg circle" was always where it was expected - on top of circle drawn with drawCircle. But unit test is failing and on golden image comparisons I can see the bottom hand is rendered a bit to the left. Is it possible that the golden image is wrong?

@spydon
Copy link
Member

spydon commented Oct 4, 2023

@spydon I checked rendering the same SVG with many different sizes (after all, that is how I found this bug) and my "svg circle" was always where it was expected - on top of circle drawn with drawCircle. But unit test is failing and on golden image comparisons I can see the bottom hand is rendered a bit to the left. Is it possible that the golden image is wrong?

Yeah, I think it is indeed wrong!
I pushed an updated goldens image (they need to be generated on linux).

@l1553k
Copy link
Contributor Author

l1553k commented Oct 4, 2023

Now I'm actually wondering where does that change in goldens come from. Previously the bottom hand was perfectly below the top one, now it's below and a bit to the left. I don't see anything in svg_test.dart test "render sharply" that should be moving the hand horizontally - no paddings, margins or whatever... any idea?

@spydon
Copy link
Member

spydon commented Oct 4, 2023

Now I'm actually wondering where does that change in goldens come from. Previously the bottom hand was perfectly below the top one, now it's below and a bit to the left. I don't see anything in svg_test.dart test "render sharply" that should be moving the hand horizontally - no paddings, margins or whatever... any idea?

Oh true, the original was correct then...
No idea what could have moved it. 🤔

@l1553k l1553k changed the title fix: Do not render SVGs bigger than requested when pixelRatio > 1 and no match in _imageCache WIP: fix: Do not render SVGs bigger than requested when pixelRatio > 1 and no match in _imageCache Oct 4, 2023
@l1553k l1553k force-pushed the main branch 5 times, most recently from 824b401 to de985b1 Compare October 5, 2023 08:19
@l1553k l1553k changed the title WIP: fix: Do not render SVGs bigger than requested when pixelRatio > 1 and no match in _imageCache fix: Do not render SVGs bigger than requested when pixelRatio > 1 and no match in _imageCache Oct 5, 2023
@spydon
Copy link
Member

spydon commented Oct 5, 2023

Now it is ready for merge right @l1553k? :)

@l1553k
Copy link
Contributor Author

l1553k commented Oct 5, 2023

I think I understand what is really going on. _render sometimes renders to a screen-bound canvas, and in that case we should just render using logical pixels and let something else (flutter/os/gpu/whatever) do it's magic. Maybe the canvas comes pre-scaled to fit the device?

And sometimes _render renders to a brand new canvas meant to be stored for later, as a bitmap I guess. In that case we need to scale the canvas to accomodate pixelRatio-as-many-pixels.

And yes, I think this is the final form :)

@spydon spydon merged commit 5fa4e09 into flame-engine:main Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants