Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[google_maps_flutter] Allow BitmapDescriptor scaling override #1737

Merged
iskakaushik merged 5 commits into from
Jun 21, 2019
Merged

[google_maps_flutter] Allow BitmapDescriptor scaling override #1737

iskakaushik merged 5 commits into from
Jun 21, 2019

Conversation

@duzenko
Copy link
Contributor

@duzenko duzenko commented Jun 13, 2019 •
edited by pedromassangocode
Loading

Description

Patch for BitmapDescriptor.fromAssetImage that respects ImageConfiguration.devicePixelRatio

Related Issues

fixes flutter/flutter#34386

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

iskakaushik
iskakaushik reviewed Jun 20, 2019
View reviewed changes
packages/google_maps_flutter/lib/src/bitmap.dart
assetName,
configuration.devicePixelRatio,
]);
}
Copy link
Contributor

@iskakaushik iskakaushik Jun 20, 2019

Choose a reason for hiding this comment

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

line 76 can be configuration.devicePixelRatio ?? assetBundleImageKey.scale

This comment has been minimized.

Sign in to view

Copy link
Contributor

@iskakaushik iskakaushik Jun 20, 2019

Choose a reason for hiding this comment

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

Makes sense.

@iskakaushik
Copy link
Contributor

iskakaushik commented Jun 20, 2019

please also change the pubspec.yml and changelog files.

@duzenko duzenko requested a review from amirh as a code owner June 20, 2019 07:41
iskakaushik
iskakaushik reviewed Jun 20, 2019
View reviewed changes
packages/google_maps_flutter/CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
## 0.5.17+1
Copy link
Contributor

@iskakaushik iskakaushik Jun 20, 2019

Choose a reason for hiding this comment

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

Can you change this to 0.5.19 please. Thanks

This comment has been minimized.

Sign in to view

packages/google_maps_flutter/lib/src/bitmap.dart
assetName,
configuration.devicePixelRatio,
]);
}
Copy link
Contributor

@iskakaushik iskakaushik Jun 20, 2019

Choose a reason for hiding this comment

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

Makes sense.

@duzenko
Merge commit '400bc414afb1e3088ff3c59b841149d1fba1a8e1' into maps-sca…
7375645 
…ling-bitmap

# Conflicts:
#	packages/google_maps_flutter/CHANGELOG.md
#	packages/google_maps_flutter/pubspec.yaml
iskakaushik
iskakaushik approved these changes Jun 21, 2019
View reviewed changes
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

LGTM

@iskakaushik iskakaushik merged commit 70f3bf1 into flutter:master Jun 21, 2019
@duzenko

This comment has been minimized.

Sign in to view
@duzenko duzenko deleted the maps-scaling-bitmap branch June 24, 2019 09:14
@duzenko duzenko restored the maps-scaling-bitmap branch June 24, 2019 09:14
@asanoki
Copy link

asanoki commented Jul 8, 2019

This CL breaks loading assets of correct scale. It passes the path to 1.0 scaled asset, and the scale ratio to Java, where however the scale is discarded. So even if mipmaps are available, always the 1.0x asset will be loaded. Can we revert?

@duzenko

This comment has been minimized.

Sign in to view
@asanoki
Copy link

asanoki commented Jul 9, 2019

This change should only kick in when devicePixelRatio != null which is exactly the idea behind it so what's broken?

That's true, but in order to load the correct mipmap on Flutter, I can't pass null. Here's what I'm doing:

final imageConfiguration = createLocalImageConfiguration(context);
_markerIcon = await BitmapDescriptor.fromAssetImage(
    imageConfiguration, 'assets/images/marker.png');

With this CL, mipmap of scale 1.0 will be loaded, even though the imageConfiguration has scale 3.0. Something is clearly broken, as the incorrect asset is loaded when you pass a scale in ImageConfiguration.

Did you try configuration.devicePixelRatio = null?

Passing null doesn't help, as then mipmaps won't be used. Just tried. Also, not sure why would I override devicePixelRatio with null, if the one provided by Flutter's createLocalImageConfiguration is correct. It's just after this CL, the value is ignored.

@duzenko

This comment has been minimized.

Sign in to view
@duzenko duzenko mentioned this pull request Jul 9, 2019
Merged
13 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@amirh amirh Awaiting requested review from amirh

1 more reviewer

@iskakaushik iskakaushik iskakaushik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow direct control of scaling for BitmapDescriptor (Maps plugin)

4 participants

@duzenko @iskakaushik @asanoki @googlebot