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

feat: Use AndroidView overload which re-uses MapView to improve performance in lazy layouts #436

Merged

Conversation

philip-segerfast
Copy link
Contributor

@philip-segerfast philip-segerfast commented Oct 13, 2023

This PR uses an AndroidView overload which allows re-using of the underlying view. This improves performance substantially when using GoogleMap in a Lazy layout, like LazyColumn.

This PR also incorporates @bubenheimer's changes from #522.


Fixes #285, #492 🦕

This will make sure that the underlying MapView is re-used in a LazyColumn (and elsewhere) which will significantly improve scrolling performance in LazyColumn.

If this overload isn't used the MapView will be destroyed every time it leaves composition.

The MapView will still be destroyed when the parent node leaves the composition.
@google-cla
Copy link

google-cla bot commented Oct 13, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@philip-segerfast philip-segerfast changed the title Android view overload re use map view feat: Android view overload re use map view Oct 13, 2023
@philip-segerfast philip-segerfast changed the title feat: Android view overload re use map view feat: Use AndroidView overload which re-uses MapView to improve performance Oct 13, 2023
@philip-segerfast philip-segerfast changed the title feat: Use AndroidView overload which re-uses MapView to improve performance feat: Use AndroidView overload which re-uses MapView to improve performance in lazy layouts Oct 13, 2023
@philip-segerfast philip-segerfast marked this pull request as draft October 24, 2023 18:35
@kikoso
Copy link
Collaborator

kikoso commented Oct 27, 2023

Hi @philip-segerfast. As far as I read in the documentation, we need to provide a non-null value in the onReset method. This PR does not seem to include it.

@philip-segerfast
Copy link
Contributor Author

philip-segerfast commented Oct 27, 2023

Hi @kikoso, it is enough to make onReset non-null. You can verify by commenting that line out in my PR and you will see that the views are re-created when scrolling.
It still needs some more work though, mainly the GoogleMap state should be reset (for example animations). That's why I made this PR into a draft.
I'll try to make another commit or get back to you regarding this issue during the weekend.

Cheers

@philip-segerfast
Copy link
Contributor Author

Upon further examination, it appears that executing a reset within onReset may not be necessary since dispositions already clear markers and halt animations, although the underlying GoogleMap's camera position state appears to be untouched.

Some other notes:

  • The issue concerning the zoom controls is unrelated to this PR, and appears to stem from a different source. You can reproduce on the main branch by putting a bunch of GoogleMap composables in a LazyColumn.
  • Additionally, version 1.6.0-alpha08 of androidx.compose.ui:ui harbors a bug leading to a crash, hence it is advisable to avoid using it.

The PR is quite ready except for the androidx.compose.ui:ui version being in alpha.

@philip-segerfast philip-segerfast marked this pull request as ready for review October 29, 2023 13:19
@wangela wangela added the status: blocked Resolving the issue is dependent on other work. label Nov 17, 2023
@wangela
Copy link
Member

wangela commented Nov 17, 2023

We'll wait until this feature moves out of alpha and doesn't cause crashes before we merge.

@philip-segerfast
Copy link
Contributor Author

Hi, @dkhawk! I made the maps pannable in the demo if you first pan them horizontally so that you still can easily scroll the list. Is this enough for you or would you like the maps to completely intercept the touch events from the LazyColumn like in MapInColumnActivity?

…operty from MapApplier [see desc.]

- Wrap MapUpdater parameters inside a MapUpdaterState data class
- Remove MapClickListeners property from MapApplier
- Pass MapClickListeners directly to MapClickListenerUpdater
@el-qq
Copy link
Contributor

el-qq commented Jul 3, 2024

@dkhawk friendly reminder about this PR (current 6.0.0 :) )

Also, we often use GoogleMap in LazyColumn and have encountered some performance issues (like oom).
Looking forward to these edits in the hope that they will improve performance

@philip-segerfast
Copy link
Contributor Author

Surely!
I'll also make sure to fix the conflicts soon so that it's ready to be merged again.

@GuillemRoca
Copy link

Great job! We will also benefit greatly if this PR gets merged soon so looking forward to it. 👏🏼

Copy link
Collaborator

@kikoso kikoso left a comment

Choose a reason for hiding this comment

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

LGTM! This should bring some improvements in terms of performance.

@kikoso kikoso mentioned this pull request Jul 9, 2024
Co-authored-by: Enrique López Mañas <[email protected]>
@dkhawk
Copy link
Contributor

dkhawk commented Jul 9, 2024

@philip-segerfast this is a great change. Thanks so much for your contribution.

@dkhawk dkhawk merged commit 0447f43 into googlemaps:main Jul 9, 2024
8 of 9 checks passed
@philip-segerfast
Copy link
Contributor Author

Thanks, you're all welcome!

googlemaps-bot pushed a commit that referenced this pull request Jul 9, 2024
# [6.1.0](v6.0.1...v6.1.0) (2024-07-09)

### Features

* Use AndroidView overload which re-uses MapView to improve performance in lazy layouts ([#436](#436)) ([0447f43](0447f43)), closes [#522](#522)
@googlemaps-bot
Copy link
Contributor

🎉 This issue has been resolved in version 6.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@el-qq
Copy link
Contributor

el-qq commented Jul 10, 2024

Local tests show great results!

Thank you very much, @philip-segerfast

@philip-segerfast
Copy link
Contributor Author

Many thanks to @bubenheimer for reviewing and helping so much too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using the new AndroidView in 1.4.0-rc01
9 participants