Skip to content

fix: Calculate zoom ratio before rendering svg #3616

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

Merged
merged 16 commits into from
Jun 14, 2025
Merged

Conversation

vunyunt
Copy link
Contributor

@vunyunt vunyunt commented Jun 3, 2025

Description

SvgComponent currently renders in lower-than-native resolution when game.camera.viewfinder.zoom is larger than 1. This is addressed by taking a ratio of canvas.getDestinationClipBounds().size and canvas.getLocalClipBounds().size, and applying it to the existing canvas scaling.

I'm still not quite sure how well this would work with pixelRatio, but I've verified that when running the "render sharply" test which has the device pixel ratio set to 3, the canvas size ratio mentioned above is 1, and not rendering in excessive resolution.

The test uses a Transform.scale to simulate viewfinder zoom. I've also verified that when running without the patch, this test generates a blurry image:

before after
Screenshot_20250604_073801 image

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?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

Fixes #3615

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.

Looks good, just a few small comments

@vunyunt
Copy link
Contributor Author

vunyunt commented Jun 6, 2025

While trying to test different width/height scale, a bug in 0be4940 was discovered. Looking at the original code for _getImage:

  Image _getImage(Size size) {
    final image = _imageCache.getValue(size);

    if (image == null) {
      final recorder = PictureRecorder();
      final canvas = Canvas(recorder);
      canvas.scale(pixelRatio);
      _render(canvas, size);
      final picture = recorder.endRecording();
      final image = picture.toImageSync(
        (size.width * pixelRatio).ceil(),
        (size.height * pixelRatio).ceil(),
      );
      picture.dispose();
      _imageCache.setValue(size, image);
      return image;
    }
   
    return image;
  }

We can see that the unscaled size is passed into _render(canvas, size);. Hence instead of passing the scaled size into _getImage, the width and height ratio should be passed in instead, and _getImage should apply the scaled size only where pixelRatio is used. This is applied in 5a5a302.

Here's a comparison of a scaling of scaleX=3, scaleY=1.5 with and without this pr now:

before after
Screenshot_20250606_082444 Screenshot_20250606_082514

Should the test be updated to have different X and Y scaling, or should an additional test be added?

@vunyunt
Copy link
Contributor Author

vunyunt commented Jun 6, 2025

Some comments about the tests:

Golden files were mistakenly updated in 5a5a302 due to the _render function being incorrectly changed. They are restored in f97e328. I've verified that the golden file generated with render sharply after f97e328 has the same hash with the original version.

Upon further inspection, the pixelRatio in svg.dart is always set to 3 regardless of tester.view.devicePixelRatio. When I tried to set tester.view.devicePixelRatio = 1, the output is at 100x100 but pixelRatio is still set to 3 for some reason. This causes the image to come out sharp even without the patch with a 2x scale, as the image is always rendered with a 3x pixel ratio. To avoid this false pass, I've updated the test to set tester.view.devicePixelRatio = 3 explicity for now.

@spydon
Copy link
Member

spydon commented Jun 6, 2025

Upon further inspection, the pixelRatio in svg.dart is always set to 3 regardless of tester.view.devicePixelRatio. When I tried to set tester.view.devicePixelRatio = 1, the output is at 100x100 but pixelRatio is still set to 3 for some reason. This causes the image to come out sharp even without the patch with a 2x scale, as the image is always rendered with a 3x pixel ratio. To avoid this false pass, I've updated the test to set tester.view.devicePixelRatio = 3 explicity for now.

In the svg.dart constructor, change to use WidgetsBinding instead, otherwise it won't make any difference if the DPR is changed in the test, so like this:

...
import 'package:flutter/widgets.dart' hide Image; // Remember to hide the other Image type

/// A [Svg] to be rendered on a Flame [Game].
class Svg {
  /// Creates an [Svg] with the received [pictureInfo].
  /// Default [pixelRatio] is the device pixel ratio.
  Svg(this.pictureInfo, {double? pixelRatio})
      : pixelRatio = pixelRatio ??
            WidgetsBinding
                .instance.platformDispatcher.views.first.devicePixelRatio;

@vunyunt
Copy link
Contributor Author

vunyunt commented Jun 10, 2025

In the svg.dart constructor, change to use WidgetsBinding instead

Seems to work 👍, with the new testing method however, I'm not sure how to set the device pixel ratio. Currently a teardown step is added to the 'render sharply' test that sets the device pixel ratio to 1. Using resetDevicePixelRatio() doesn't seem to work (stays at 3).

@spydon
Copy link
Member

spydon commented Jun 10, 2025

Seems to work 👍, with the new testing method however, I'm not sure how to set the device pixel ratio. Currently a teardown step is added to the 'render sharply' test that sets the device pixel ratio to 1. Using resetDevicePixelRatio() doesn't seem to work (stays at 3).

Just set the device pixel ratio inside of the test, or in the setup function. The tear down is called after each test has run.

@vunyunt
Copy link
Contributor Author

vunyunt commented Jun 10, 2025

Seems to work 👍, with the new testing method however, I'm not sure how to set the device pixel ratio. Currently a teardown step is added to the 'render sharply' test that sets the device pixel ratio to 1. Using resetDevicePixelRatio() doesn't seem to work (stays at 3).

Just set the device pixel ratio inside of the test, or in the setup function. The tear down is called after each test has run.

Ah, I'm not sure how should this be done under either setUp() or testGolden()? The render sharply test seems to do this by setting the view.devicePixelRatio field under the WidgetTester instance, but testGolden seems to provide a game instance instead of the WidgetTester instance. To clarify, the cleanup step is added to the original render sharply test and not the newly added Svg.withCameraZoom test.

My assumption is that the reason it's set to 3 with the new test is because of tester.view.devicePixelRatio = 3 in the render sharply test. I got the impression that it's affecting the new test from this function description of WidgetTester.binding.setSurfaceSize: To avoid affecting other tests by leaking state, a test that uses this method should always reset the surface size to the default. For example, using addTearDown:

@spydon
Copy link
Member

spydon commented Jun 10, 2025

@vunyunt ah right, I forgot that you can't access the tester from within testGoldens (we should probably fix that), but for now, let's just use the old method for this test then.

@vunyunt
Copy link
Contributor Author

vunyunt commented Jun 12, 2025

Alright, the test has been restored to just before 85e885b

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.

Thanks for your contribution!

@spydon spydon enabled auto-merge (squash) June 14, 2025 15:15
@spydon spydon merged commit f8b7ef8 into flame-engine:main Jun 14, 2025
8 checks passed
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.

SvgComponent blurred when camera.viewFinder.zoom is larger than 1
2 participants