[google_maps_flutter] Ground overlay support - platform impls #8563
Conversation
github-actions
bot
added
p: google_maps_flutter
platform-android
platform-ios
platform-web
labels
jokerttu
force-pushed
the
feat/google_maps_flutter_ground_overlay_support_platform_impl
branch
from
a859a4d to
43522ae
Compare
stuartmorgan-g
added
triage-ios
|
Looking at web stuff! |
stuartmorgan-g
removed
triage-ios
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
Android and iOS LGTM; I did a full review of both. Please wait for @ditman's review covering web before landing though.
There was a problem hiding this comment.
The return and the parameters should get nullability annotations.
There was a problem hiding this comment.
This can be pulled out of the other file into a test helper file, rather than duplicating the code.
There was a problem hiding this comment.
Moved to TestImageUtils.java
There was a problem hiding this comment.
Does nullable instancetype not work? (I can never remember the exact set of cases where nullable doesn't work).
There was a problem hiding this comment.
I removed the nullable annotation from these instance initialisations as they do not explicitly return nil.
There isn't any handling for null instances on code either.
I would assume that self = [super init]; returns instance, and if not, something is really broken.
There was a problem hiding this comment.
Nit: If you make locals for groundOverlay.position and groundOverlay.bounds just before the if block, and use the locals everywhere after that (including the conditions), you won't need most of the force-unwraps below.
|
(@ash-google Feel free to review as well, I just had already looked over the mobile platforms in the main PR so I went ahead and did a review here to cover those.) |
There was a problem hiding this comment.
This should have an assertion message indicating why it is required or at least log an error saying that width is required.
There was a problem hiding this comment.
Can we extract these constants into local constants with good naming so it's easier to understand why we need these magic numbers?
There was a problem hiding this comment.
@ash-google is this what you were looking for? cdf4d25
|
Did a second look over Android and LGTM. Let's wait for @ditman for web. |
|
There is some crash issues on Android integration tests: Linux_android android_device_tests_shard_1 master |
It's flutter/flutter#159731. We're seeing flare-ups of this on other PRs that touch maps, so I don't think it's related. We've seen spikes in this crash in our CI before, potentially due to server-side changes in Google Maps. |
|
I tested this implementation a bit on Android. Unfortunately, I ran into an issue which occurs when ground overlay images change quickly (<250ms interval). This results in a quick but noticeable black flicker. The images are stored in-memory as The I am not sure what causes this, maybe the thread that updates the ground overlays doesn't manage to decode bitmaps from raw data quickly enough? I attached a video demonstrating this on Xiaomi 12T. Maybe I could try creating minimal reproducible example if needed. out.mp4 |
There was a problem hiding this comment.
By default asserts are stripped in java. If you want to check this value then you will need to throw an exception or log or do some other form of error handling.
There was a problem hiding this comment.
Can you add a link to what layer of the stack does not support this functionality? Also why is it safe to use a dummy image?
There was a problem hiding this comment.
This is used only by inpector api, for testing purposes. App facing widget stores groundoverlays on dart implementation, and there is no real reason to fetch the data back, other than testing.
Image is mandatory field on PlatformGroundOverlay (and it should be kept non-nullable), therefore image must be set for the object.
The image is description either contains set of bytes, or path to asset. This info is converted to format google maps uses (BitmapDescription), and the original data is not stored on native code.
So converting read and possibly rescaled asset bitmap back orginal instruction with asset url is not currently possible without storing the original PlatformImageDescriptor to memory; which is not optimal.
Therefore using dummy image here should be fine?
There was a problem hiding this comment.
Thanks this helps answer my question. Consider putting a shorter version of that in the code.
Maybe something like "Image is mandatory field on PlatformGroundOverlay (and it should be kept non-nullable), therefore image must be set for the object. The image is description either contains set of bytes, or path to asset. This info is converted to format google maps uses (BitmapDescription), and the original data is not stored on native code."
There was a problem hiding this comment.
Consider extracting a helper method that takes a actual ground overlay object and expected values and handles the assertions for equals. It would make new tests easier to write/review.
There was a problem hiding this comment.
Can you add a comment for why this value was chosen?
There was a problem hiding this comment.
Did you mean to skip this for all platforms? if so should it be deleted instead?
There was a problem hiding this comment.
No, this was committed by accident. Not related to this PR.
There was a problem hiding this comment.
This is a legacy of this file starting its life as a copy of the integration tests in google_maps_flutter. In the context of google_maps_flutter_android, Platform.isAndroid is just a confusing way of writing true, so this change was good.
if so should it be deleted instead?
I would prefer we keep it, because of the TODO above. We are starting to see features that do not work without an API key, so we will need to fix the CI limitation at some point. (I was working on it with infra folks a while ago, but that got derailed.)
There was a problem hiding this comment.
This is true; but already reverted this change before I saw @stuartmorgan:n comment about this, as this was chore change, not related to ground overlay at all. I can commit this back, if it is ok to have unrelated changes in the PR as well.
There was a problem hiding this comment.
It's fine either way; if you've already reverted it don't worry about adding it back unless you feel like it.
There was a problem hiding this comment.
Can you add a comment explaining where these magic numbers come from?
I believe this is just bumping the bearing 10 degrees and making sure we dont go over 360 but that is after reading the whole pr.
There was a problem hiding this comment.
|
I'll address review comments made to main PR by @reidbaker later this week: #8432 (review) |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I took a look at web since I'm not sure when @ditman will have time. Mostly it seems straightforward, but I have a couple of questions.
There was a problem hiding this comment.
The other types use a nullable gmaps object, and remove() also nulls it out. What's the reason for the different pattern here?
There was a problem hiding this comment.
Why is this is-checking one type and then as-casting to a different type?
jokerttu
force-pushed
the
feat/google_maps_flutter_ground_overlay_support_platform_impl
branch
from
29a8e41 to
f482577
Compare
jokerttu
force-pushed
the
feat/google_maps_flutter_ground_overlay_support_platform_impl
branch
from
388c1c3 to
e89bada
Compare
|
@reidbaker are you going to check the changed made addressing these #8432 (review) (on this PR and also mirrored to original app facing PR) |
|
Thanks for asking, @ash2moon's approval for android is good enough for me. Thank you for this change. |
jokerttu
added
the
autosubmit
flutter/packages@d450e1b...dd781d4 2025-03-19 [email protected] [camera_avfoundation] Tests backfilling - part 4 (flutter/packages#8854) 2025-03-18 [email protected] [camera_avfoundation] Tests backfilling - part 5 (flutter/packages#8873) 2025-03-18 [email protected] [video_player]: reduce video player position update interval from 500ms to 100ms (flutter/packages#8346) 2025-03-18 [email protected] [google_maps_flutter] Support for Ground Overlay (flutter/packages#8432) 2025-03-18 [email protected] [google_maps_flutter] Ground overlay support - platform impls (flutter/packages#8563) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#8563) This PR contains platform implementations for the ground overlays support (flutter#8432). Linked issue: flutter/flutter#26479
…r#165489) flutter/packages@d450e1b...dd781d4 2025-03-19 [email protected] [camera_avfoundation] Tests backfilling - part 4 (flutter/packages#8854) 2025-03-18 [email protected] [camera_avfoundation] Tests backfilling - part 5 (flutter/packages#8873) 2025-03-18 [email protected] [video_player]: reduce video player position update interval from 500ms to 100ms (flutter/packages#8346) 2025-03-18 [email protected] [google_maps_flutter] Support for Ground Overlay (flutter/packages#8432) 2025-03-18 [email protected] [google_maps_flutter] Ground overlay support - platform impls (flutter/packages#8563) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#8563) This PR contains platform implementations for the ground overlays support (flutter#8432). Linked issue: flutter/flutter#26479
…r#165489) flutter/packages@d450e1b...dd781d4 2025-03-19 [email protected] [camera_avfoundation] Tests backfilling - part 4 (flutter/packages#8854) 2025-03-18 [email protected] [camera_avfoundation] Tests backfilling - part 5 (flutter/packages#8873) 2025-03-18 [email protected] [video_player]: reduce video player position update interval from 500ms to 100ms (flutter/packages#8346) 2025-03-18 [email protected] [google_maps_flutter] Support for Ground Overlay (flutter/packages#8432) 2025-03-18 [email protected] [google_maps_flutter] Ground overlay support - platform impls (flutter/packages#8563) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
6 participants
This PR contains platform implementations for the ground overlays support (#8432).
Linked issue: flutter/flutter#26479
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.