Skip to content

feat(mobile): drift map page #19898

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat(mobile): drift map page #19898

wants to merge 10 commits into from

Conversation

wuzihao051119
Copy link
Collaborator

@wuzihao051119 wuzihao051119 commented Jul 12, 2025 •
edited
Loading

We need to refactor timeline and bottom sheet to display timeline in the bottom sheet.

  • new map page without api calls
  • a universal map component like timeline. It can be used in any page.

There is much higher efficiency than previous map widget.

@wuzihao051119 wuzihao051119 marked this pull request as ready for review July 12, 2025 20:07
@alextran1502
Copy link
Member

alextran1502 commented Jul 13, 2025 •
edited
Loading

Testing with an instance of 80,000+ assets, it triggered an error while I am moving around the map. Below is the logs

I/flutter (27422): FlutterError - Catch all: SqliteException(1): while preparing statement, too many SQL variables, SQL logic error (code 1)
I/flutter (27422):   Causing statement (at position 98484): SELECT COUNT("remote_asset_entity"."id") AS "c0", DATE(datetime("remote_asset_entity"."created_at", 'localtime')) AS "c1" FROM "remote_asset_entity" WHERE "remote_asset_entity"."id" IN (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?
I/flutter (27422):  package:sqlite3/src/implementation/exception.dart 87                                 throwException
I/flutter (27422): package:sqlite3/src/implementation/database.dart 377                                 DatabaseImplementation._prepareInternal
I/flutter (27422): package:sqlite3/src/implementation/database.dart 436                                 DatabaseImplementation.prepare
I/flutter (27422): package:sqlite3/src/ffi/implementation.dart 117                                      FfiDatabaseImplementation.prepare
I/flutter (27422): package:drift/src/sqlite3/database.dart 184                                          Sqlite3Delegate._getPreparedStatement
I/flutter (27422): package:drift/src/sqlite3/database.dart 158                                          Sqlite3Delegate.runSelect
I/flutter (27422): package:drift/src/runtime/executor/helpers/engines.dart 78                           _BaseExecutor.runSelect.<fn>
I/flutter (27422): package:drift/src/runtime/executor/helpers/engines.dart 62                           _BaseExecutor._synchronized
I/flutter (27422): package:drift/src/runtime/executor/helpers/engines.dart 75                           _BaseExecutor.runSelect
I/flutter (27422): package:drift/src/remote/server_impl.dart 163                                        ServerImplementation._runQuery
I/flutter (27422): package:drift/src/remote/server_impl.dart 119                                        ServerImplementation._handleRequest.<fn>
I/flutter (27422): package:drift/src/remote/communication.dart 165                                      DriftCommunication.setRequestHandler.<fn>
I/flutter (27422): ===== asynchronous gap ===========================
I/flutter (27422): package:drift/src/remote/communication.dart 113                                      DriftCommunication.request
I/flutter (27422): package:drift/src/remote/client_impl.dart 101                                        _BaseExecutor._runRequest
I/flutter (27422): package:drift/src/remote/client_impl.dart 141                                        _BaseExecutor.runSelect
I/flutter (27422): package:drift/src/utils/lazy_database.dart 96                                        LazyDatabase.runSelect
I/flutter (27422): package:drift/src/runtime/query_builder/statements/select/select_with_join.dart 446  JoinedSelectStatement._getRaw.<fn>
I/flutter (27422): package:drift/src/runtime/api/connection_user.dart 171                               DatabaseConnectionUser.doWhenOpened.<fn>
I/flutter (27422): dart:async/zone.dart 1538                                                            _rootRunUnary
I/flutter (27422): package:drift/src/runtime/cancellation_zone.dart 65                                  NonNullableCancellationExtension.resultOrNullIfCancelled
I/flutter (27422): package:drift/src/runtime/executor/stream_queries.dart 341                           QueryStream.fetchAndEmitData
I/flutter (27422):

While loading, the map is quite janky, fps drops significantly

@wuzihao051119
Copy link
Collaborator Author

wuzihao051119 commented Jul 13, 2025 •
edited
Loading

This error has been fixed. But we should refactor the timeline. When ProviderScope changed, timeline should be refreshed.

In my phone, the fps is up to 120+ theoretically. It is so fluent. No any jank.

image

@shenlong-tanwen
Copy link
Member

This error has been fixed. But we should refactor the timeline. When ProviderScope changed, timeline should be refreshed.

You can assign the bounds as the key to the Timeline widget and it should reload whenever the bounds are updated

alextran1502
alextran1502 reviewed Jul 15, 2025
View reviewed changes
alextran1502
alextran1502 reviewed Jul 15, 2025
View reviewed changes
alextran1502
alextran1502 reviewed Jul 15, 2025
View reviewed changes
mobile/lib/infrastructure/repositories/timeline.repository.dart
LatLngBounds bounds, {
GroupAssetsBy groupBy = GroupAssetsBy.day,
}) {
if (groupBy == GroupAssetsBy.none) {
Copy link
Member

Choose a reason for hiding this comment

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

probably remove this condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cannot understand. Can you explain the meaning?

@alextran1502
Copy link
Member

Which instance do you use to test? I tested on my stress test instance and moving around the map is very janky, in production build

@wuzihao051119
Copy link
Collaborator Author

wuzihao051119 commented Jul 15, 2025 •
edited
Loading

I test in https://main.preview.internal.immich.cloud. Would you mind provide an instance for me? I don't have so many assets in my server. I think removing layers, adding sources and add layers is the cause, so I try to optimize it.

@alextran1502
Copy link
Member

I can't unfortunately, it is a clone of my personal instance

@wuzihao051119
Copy link
Collaborator Author

Ok, I will optimize first, and then you can test and give feedback.

shenlong-tanwen
shenlong-tanwen reviewed Jul 15, 2025
View reviewed changes
shenlong-tanwen
shenlong-tanwen reviewed Jul 15, 2025
View reviewed changes
shenlong-tanwen
shenlong-tanwen reviewed Jul 15, 2025
View reviewed changes
shenlong-tanwen
shenlong-tanwen reviewed Jul 15, 2025
View reviewed changes
@wuzihao051119
Copy link
Collaborator Author

wuzihao051119 commented Jul 16, 2025 •
edited
Loading

@alextran1502 Can you test in release mode again? I think the performance issue has been solved.

shenlong-tanwen
shenlong-tanwen reviewed Jul 16, 2025
View reviewed changes
mobile/lib/presentation/widgets/map/map.widget.dart Outdated
Comment on lines 180 to 192
final bounds = ref.watch(mapStateProvider.select((s) => s.bounds));
AsyncValue<Map<String, dynamic>> markers =
ref.watch(mapMarkerProvider(bounds));
AsyncValue<Map<String, dynamic>> allMarkers =
ref.watch(mapMarkerProvider(null));

ref.listen(mapStateProvider, (previous, next) async {
markers = ref.watch(mapMarkerProvider(bounds));
});

markers.whenData((markers) => reloadMarkers(markers));
allMarkers
.whenData((markers) => reloadMarkers(markers, isLoadAllMarkers: true));
Copy link
Member

@shenlong-tanwen shenlong-tanwen Jul 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

These could actually be part of the parent widget and we could remove this private widget all together. It feels weird to return the bottom sheet widget from a widget called _Markers.

Also, why'd we have two different marker providers? Is the idea to load only those displayed in the current bounds until all the markers for the maps are loaded, after which, we don't make any more marker updates? If so, the current code is a bit incorrect in the sense that it still makes redundant DB calls whenever the bounds are updated, but the results are ignored after loadAllMarkers is set to true

You can refactor all this logic into a single provider and address all these issues more efficiently.

Copy link
Collaborator Author

@wuzihao051119 wuzihao051119 Jul 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, if you move the map, the markers in the bounds will be loaded. At the same time, all markers will be preloaded. When all the markers are loaded, we don't need to load or filter markers any more.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if you move the map, the markers in the bounds will be loaded. At the same time, all markers will be preloaded. When all the markers are loaded, we don't need to load or filter markers any more.

Yeah, we can do this in a single provider and make things simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have deleted redundant DB calls. Let's see the performance. And then refactor it.

@alextran1502
Copy link
Member

Hello, another round of testing. I see the improvement, I can move the map around, zoom in and out. However, the framerate is still relatively low and janky. I think there are still some bottle neck of there might be place we can debounce better

@wuzihao051119
Copy link
Collaborator Author

wuzihao051119 commented Jul 17, 2025 •
edited
Loading

Can you record the stack by android and flutter profile tool and provide me with flame chart? I want to confirm if it is a rendering issue with OpenGL because now we do not have any db calls and markers reload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shenlong-tanwen shenlong-tanwen shenlong-tanwen left review comments

@alextran1502 alextran1502 Awaiting requested review from alextran1502

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
changelog:skip 📱mobile
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wuzihao051119 @alextran1502 @shenlong-tanwen