[google_maps_flutter] Custom marker size improvements

[jokerttu]

This PR adds and improves marker scaling support for Android, iOS and Web. With this change, custom marker icons are drawn with same logical size on all platforms, keeping the visual style as close as possible.

This PR is created from previous PR: https://github.com/flutter/plugins/pull/6805

To avoid breaking change, this PR makes the following changes while keeping the old behavior intact:

• Deprecates BitmapDescriptor.fromAssetImage in favor of BitmapDescriptor.createFromAsset
• Deprecates BitmapDescriptor.fromBytes in favor of BitmapDescriptor.createFromBytes

Android:

Web:

Resolves https://github.com/flutter/flutter/issues/34657 Resolves https://github.com/flutter/flutter/issues/70923

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• [x] I signed the CLA.
• [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• [x] I listed at least one issue that this PR fixes in the description above.
• [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] I added new tests to check the change I am making, or this PR is test-exempt.
• [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[ilDeffo]

Any update on when this will be merged?

[stuartmorgan-g]

@cyanglaz, @ditman, @reidbaker: ping on the platform portions of this review.

[gmackall]

@jokerttu It looks like there are still some outstanding comments on this PR, is this something you are still actively working on?

[jokerttu]

@jokerttu It looks like there are still some outstanding comments on this PR, is this something you are still actively working on?

Yes, I am actively working on addressing the outstanding feedback. I will make the necessary updates to the PR in the near future.

[camsim99]

I am marking the PR as a Draft to remove it from our queue. When you're actively working on this again, please feel free to mark the PR as ready for review.

[goderbauer]

(triage): @jokerttu Do you still have plans to come back to this PR in the near future?

[jokerttu]

@goderbauer

(triage): @jokerttu Do you still have plans to come back to this PR in the near future?

I am currently rewriting updated version following the request from @ditman https://github.com/flutter/packages/pull/4055/files#r1326628458 Revisited version coming soon.

[jokerttu]

I'm currently focusing on updating test cases for the web implementation, following the implementation of automatic scaling. Related to these changes discussed here using HTMLImageElement to load image size on web.

I am also updating the constructors for BytesMapBitmap and AssetMapBitmap:

• For AssetMapBitmap, it will no longer require ImageConfiguration since this is primarily needed for mipmapping instructions. In scenarios where developers use assets without specifying an pixel ratio, it's generally preferable to default to an imagePixelRatio of 1.0. And developers need to instructed to give properly sized images for different

• Similarly, BytesMapBitmap will default to an imagePixelRatio of 1.0, moving away from using the device's pixel density as the default setting. Developers aiming for higher resolution images should explicitly set the imagePixelRatio parameter or size parameter based on the desired aspect ratio. Defaulting images pixel ratio to devices pixel density, would change the marker size depending on the device, and would not have consistent results.

These updates are intended to enhance the usability and flexibility of these newly introduced classes.

I've converted this pull request to a draft, as I won't be able to complete this work today.

[jokerttu]

@ditman automatic marker icon scaling without given size is now implemented for web as well using the onLoad event of HTMLImageElement. This implementation is visible in commit: https://github.com/flutter/packages/pull/4055/commits/3c2388832b519cb22d79c64d90374a571fb40250

@stuartmorgan Updated the documentation and constructors for AssetMapBitmap and BytesMapBitmap in commit: https://github.com/flutter/packages/pull/4055/commits/72f889eb23fad5ebcfc8aed77573279cd65dee15

Please review the documentation changes for these classes and methods as well.

As this PR also might cause upscaling on low-res images to keep the logical size same between the devices, it is important to have good documentation to have information how to control the sizing, with BitmapScaling.noScaling or with the imagePixelRatios or size parameters, which all give now really good and consistent control of the marker sizes between platforms.

[ditman]

it is important to have good documentation to have information how to control the sizing, with BitmapScaling.noScaling or with the imagePixelRatios or size parameters,

Flutter is able to pick the correct "Asset" at the right dpi, if it exists, from the defined assets (Resolution-aware image assets).

If possible, AssetMapBitmap should build off of that, right? (Then BytesMapBitmap would need "extra help" by specifying the dpi of each asset?)

[jokerttu]

it is important to have good documentation to have information how to control the sizing, with BitmapScaling.noScaling or with the imagePixelRatios or size parameters,

Flutter is able to pick the correct "Asset" at the right dpi, if it exists, from the defined assets (Resolution-aware image assets).

If possible, AssetMapBitmap should build off of that, right? (Then BytesMapBitmap would need "extra help" by specifying the dpi of each asset?)

it is important to have good documentation to have information how to control the sizing, with BitmapScaling.noScaling or with the imagePixelRatios or size parameters,

Flutter is able to pick the correct "Asset" at the right dpi, if it exists, from the defined assets (Resolution-aware image assets).

If possible, AssetMapBitmap should build off of that, right? (Then BytesMapBitmap would need "extra help" by specifying the dpi of each asset?)

@ditman yes, but the current version also upscales assets if not given with proper dpi, and I think this need to be revisited and changed.

This PR already fixes a lot of issues, for example adds way to give logical size for assets that allows to render them wirh same exact logical size on each device and platform.

Assets should always been drawn pixel-by-pixel by default, so current behaviour needs to be changed. Current approach uses image pixelratio on native to draw the 30x30 image with pixelratio 1.0 as big as 90x90 with pixelratio 3.0. Wirh this (currrent) approach the assets are by default drawn at same logical size on each device but this also causes upscaling, and visible pixels.

Aiming to render each asset automatically same logical size on each device is not the right default, and it should be done only with the size parameter, and automatic upscaling should be removed, as developers should be able to by default render the images on their natural resolution. Not sure if there is place for this upscaling at all. This of course means that same asset bitmap will be smaller on higher dpi displays, but developers should use mipmaps to mitigate this issue (or size parameter). Link to the doc for resolution aware assets should be added to the dartdocs? Might be that imagePixelRatio is not even needed on native side on any platform (I'll check if this can be dropped out)

Current approach follows how UIImage scale parameter works on iOS, where scale tells the images pixel ratio, causing it to be rendered with different size on the screen depending of the given value. The implementation should not change this value, and it should be kept as default (devices pixel ratio) for each UIImage.

For BytesMapBitmap the case not so simple. These assets could be for example images loaded from net. Developers should have way to scale these easily as they might not have access to different bitmaps for different dpi's. ImagePixelRatio might not be proper parameter for this. Instead this should have scale parameter which can be used to upscale (or downscale) these bitmaps. This allows them to use the devices dpi as scale to keep the asset upscaled to same size on each device. Developers can use size parameter as well.

So, my suggestion is that I do following changes:

• I remove automatic upscaling to dpi by default (on iOS UIImage scale should be kept as default)
• imagePixelRatio parameter should be replaced with scale parameter for both AssetMapBitmap and BytesMapBitmap allowing developers to easily upscale (or downscale) bitmaps if they want without knowing the images size. Scale is by default 1.0.
• BitmapScaling enum could be removed as there is now automatic scaling.
• Tests and documentation updates

I think the updated behaviour should be better and simpler, eventhough this change would mean that the asset size would differ wirh different dpi's but as said, sharp rendering is more important, and developer always can use size parameter (or scale) if they want.

On web the image width and height need to be always devided with devices pixel ratio as it render everything too big by default on high dpi displays; this is easy to fix now when we have way to detect image size on the web plstform.

Sorry for the tuning. This change should not take long to fix as this is quite small change and won't cause big changes on native code either.

[ditman]

On web the image width and height need to be always devided with devices pixel ratio as it render everything too big by default

More precisely, the web uses the "logical size" of an asset (physical size / dpr) for everything, but the images need to be constructed at the "physical size".

For a web developer, the weirdness is having to create their asset at 2x when DPI is > 1 (see this recent issue, for example!)

If you update the docs more, please describe how web devs can have sharp images (I think the web is better at downscaling than upscaling, but even then, having the right size for the current screen DPI is most important)

((Best image size for the current DPI == what iOS calls mip-mapping?))

((PS: thanks for taking care of this, it's a big source of issues for web users!))

[jokerttu]

@ditman

On web the image width and height need to be always devided with devices pixel ratio as it render everything too big by default

More precisely, the web uses the "logical size" of an asset (physical size / dpr) for everything, but the images need to be constructed at the "physical size".

Yes, exactly,

I was wrong with this approach https://github.com/flutter/packages/pull/4055#issuecomment-1998720327 as it would be more problematic for developers if the assets are renderer in different logical size on different devices by default.

Currently implemented behaviour works same way as flutter is rendering assets with certain scales (scale is same as imagePixelRatio). If developers only give asset with imagePixelRatio/scale of 1.0 it is rendered upscaled on flutter if devices pixel ratio is >1, and the current approach matches this behaviour for markers. For BitmapScalingMethod, current behaviour give two options, auto and noScaling where noScaling means that the images are drawn directly on platform without scaling, and this causes some issues. On iOS and android images are drawn pixel-by-pixel but as you said, on web the image is drawn differently, it always scales the images with the device pixel ratio by default (and this is understandable as all websites needed to work with first retina displays as same size as 1.0 pixel ratio displays). With current approach this means that if developers want to render the image pixel-by-pixel, the only way to do this on all platforms is to give the devices pixel ratio as a value for imagePixelRatio.

I am trying to find best API for scaling, and might rename the noScaling as platformDefault, as this describes better what is happening (like web really does scaling in this situation). This option is given as it is more performant option if performance need to be improved by certain use cases.

I'm also changing how logical size is given for the bitmaps (assets and bitmaps), as it currently does not preserve aspect ratio. Instead of size, developers should be able to give optional width and height parameters. If only width is given, images are resized to this and aspect ratio is preserved and wise versa. If both are given the image is rendered using the exact size given. Flutter ResizeImage class uses following policy https://api.flutter.dev/flutter/painting/ResizeImagePolicy.html, and I am going to follow the exact policy with marker image scaling. The fit policy will not be implemented, but it can be added quite easily with separate "MapBitMapResizePolicy" enum holding exact and fit policies if needed.

For a web developer, the weirdness is having to create their asset at 2x when DPI is > 1 (see this recent issue, for example!)

Developers should always provide the assets/images for certain device dpi:s. This PR already unifies how this can be controlled with imagePixedRatio and size (note: size will be replaced with width and height) on each platform, meaning that on https://github.com/flutter/flutter/issues/143398 the issue can be solved only by providing proper imagePixelRatio for the asset and it should be rendered automatically with proper logical size. For web, it should be recommended to always give assets and bitmaps at 2.0 scale/imagePixelRatio, as then they will be always sharp on all device pixel ratios.

If you update the docs more, please describe how web devs can have sharp images (I think the web is better at downscaling than upscaling, but even then, having the right size for the current screen DPI is most important)

The most important thing is to improve the docs, and maybe even highlight separately how web markers images should be provided. Developers could also provide assets also (or only) in 2.0x folder to ensure sharp rendering on each platform. For image generation the device pixel ratio should be used to render properly scaled image.

((Best image size for the current DPI == what iOS calls mip-mapping?))

((PS: thanks for taking care of this, it's a big source of issues for web users!))

PS. when flutter is finding proper asset size for device pixel ratios higher than 1 but less than 2 (>1, <=2), it will always select 2, as upscaling from 1 to for example 1.4 does not look good. On higher scales the closest is selected.

[jokerttu]

Sorry for a long period from last update on this. Refactored the behaviour to match the review comments given in history.

One of the main concerns was the sizing without preserving image aspect ratio, this is now fixed as instead of size, users can now init AssetMapBitmap and BytesMapBitmap with optional width and height parameters. If only one of these are given the image is scaled preserving its aspect ratio on each platform. Also imagePixelRatio is supported for both types (used if width and height is not given).

Serialization for asset and bytes BitmapDescription types are updated for cleared parser on native Android and iOS implementations.

Updated example apps to allow testing the assets with only width or height or both.

Added helper wrapper methods to the BitmapDescriptor class.

Hopefully this is now better and clearer API for the developers.

[jokerttu]

@hellohuanlin

Could you address my previous comments that's not addressed yet?

Sorry, missed some earlier; should be now fixed but this one is still open for discussion: https://github.com/flutter/packages/pull/4055#discussion_r1568646976

Also notice that the logic for resizing is changed from optional size to optional width and height values allowing easier way to preserve aspect ratio.

[reidbaker]

@jokerttu is this ready for a another pass from me?

[jokerttu]

As platform interface changes are already landed (https://github.com/flutter/packages/pull/6687), deprecating old methods, I have went forward on creating PR for platform implementations of this federated plugin here: https://github.com/flutter/packages/pull/6826.

@reidbaker could you continue the process on this PR.

[jokerttu]

Converted as draft to avoid unnecessary reviews on this PR before platform implementations are landed. See the platform implementation portion of this PR here: https://github.com/flutter/packages/pull/6826

This PR will be updated and marked ready for review with only app-facing plugin changes after the platform impls are landed.

[jokerttu]

This PR now contains only the app-facing api changes in google_maps_flutter package.

@stuartmorgan what would be correct process to deprecate the old methods under BitmapDescriptor class.