mapsplugin / cordova-plugin-googlemaps

Google Maps plugin for Cordova
Apache License 2.0
1.66k stars 918 forks source link

Performance problems when adding many markers. #835

Closed atannus closed 8 years ago

atannus commented 8 years ago

I'm proposing a patch to the PluginMarker.java file that will greatly improve the performance when adding hundreds of markers to the map.

I propose the creation of a createMarkers method (note the plural), which will take the same marker definition as does the currently available createMarker, except for the fact they'll be in an array, therefore allowing the creation of multiple markers with a single over-the-bridge request. Naturally, the callback has to return an array of markers, instead of a single marker.

A private createMarker_ method is to be called by createMarkers. This does not break the current API, and the current createMarker method can be refactored to simply call the private version with its argument in an array.

I have already implemented marker caching and marker pre-loading which on top of that will drastically reduce memory consumption and prevent leaks.

This is where my understanding of this project ends. I don't know what my constraints as a developer are:

I'm seeing fantastic performance with these improvements, I'd really like to contribute them.

Thanks a lot for the help.

wf9a5m75 commented 8 years ago

I've been working on the multiple maps continueously. I guess the most of methods are worked without problem on iOS(not Android), but I've been testing one by one. If you are interested in it, try the multiple_maps branch (works only for iOS, Android is not ready yet even for testing)

cvaliere commented 8 years ago

thanks! so, if I sum up:

I guess that branch "multiple_maps" may change the API, though? Until now, if we make multiple calls to method getMap, we receive the same instance of map, but I guess it won't be the case anymore?

wf9a5m75 commented 8 years ago

The getMap() method always return a new instance of a map in the multiple_maps branch. And the multiple_maps branch iherites the optimization branch, but I've been adjusting the multiple maps action especially on the iOS.

cvaliere commented 8 years ago

OK, so we have to change our JS code a bit, because until there, when we called getMap(), it was always the same instance, so there was no need to redefine events listeners for instance

The problem is that we need to have a common API for Android & iOS, so I will wait until multiple maps is ready for both platforms

Any ETA?

wf9a5m75 commented 8 years ago

No ETA, sorry. I was busy until 3 days ago (since my mother was died...)

hirbod commented 8 years ago

My sincere condolences. Take all the time you need

cvaliere commented 8 years ago

My condolences too If you want to stop for a while, which would be totally understandable, please just provide what's left to develop on Android, and I'll develop it

celron commented 8 years ago

I'm sorry to hear about your loss @wf9a5m75

cvaliere commented 8 years ago

my usual ping current situation is that

and it's been 5 months now please tell us if you need help

cvaliere commented 8 years ago

@wf9a5m75 6 months same situation help still available if you decide to value teamwork

wf9a5m75 commented 8 years ago

@cvaliere Thank you for waiting long time.

As you may know, I don't stop at all. I have still working on many things little by little. You can catch up what I do. https://github.com/mapsplugin/cordova-plugin-googlemaps/commits/multiple_maps

So what's now. I'm focusing on the multiple maps with multiple pages. This is an example apk (alpha version level). (apk) https://drive.google.com/open?id=0B1ECfqTCcLE8QVBWNl9pd2lyX00 (youtube) https://www.youtube.com/watch?v=Insz0tPCB44

The reason of what I don't receive your help(s) is that I have been strengthening the foundation of the plugin. In order to improve performance, saving memory, and use multiple maps, The codes of Javascript and the native side are becoming stable, but not stable, still changing.

In order to work with the team, I need to make the load map, but since the unexpected things are still happen some time, not yet the time. Please wait. (However I have been preparing it)

Regarding of the multiple_maps branch, I'm working on the map and the marker classes only. If I feel the two classes are stable, I will ask to help for other classes such as Polyline, Polygon...etc.

You can test the branch, but no guarantee, no crams please.

cvaliere commented 8 years ago

thank you for your answer I indeed see that you make progress, but sadly, I must use this new branch: performance on master is awful, and I do need multiple maps

We are desperately trying to use branch multiple_maps, but we are stuck. We made a fork here: https://github.com/elpsk/cordova-plugin-googlemaps/commits/multiple_maps, and we already fixed some things, but we still have an enormous issue on iOS, with the map being hidden below all other elements

guillenotfound commented 8 years ago

@cvaliere Is that branch working on Android?

cvaliere commented 8 years ago

dunno yet, we are trying to make it work on iOS just creating the map and putting a marker on it, and it doesn't work... I'm desperate

wf9a5m75 commented 8 years ago

Um, did you install this https://github.com/apache/cordova-plugins/tree/master/wkwebview-engine-localhost ?

cvaliere commented 8 years ago

nope should I?

wf9a5m75 commented 8 years ago

Yes, at this time. In the feature, you will not need to install (probably). And, the wkwebview-engine-localhost plugin inserts <content src="http://localhost:0" /> under <widget> tag, but this is a problem for Android. So you need to modify the config.xml like below.

<?xml version='1.0' encoding='utf-8'?>
<widget id="cordova.google.maps" version="0.0.1" xmlns="http://www.w3.org/ns/widgets" xmlns:cdv="http://cordova.apache.org/ns/1.0">
    <name>HelloCordova</name>
    <description>
        A sample Apache Cordova application that responds to the deviceready event.
    </description>
    <author email="dev@cordova.apache.org" href="http://cordova.io">
        Apache Cordova Team
    </author>
    <plugin name="cordova-plugin-whitelist" spec="1" />
    <access origin="*" />
    <access origin="*" />
    <platform name="android">
        <allow-intent href="market:*" />
    </platform>
    <platform name="ios">
        <preference name="AlternateContentSrc" value="http://localhost:0" />
        <content src="http://localhost:0" />
        <allow-intent href="itms:*" />
        <allow-intent href="itms-apps:*" />
    </platform>
</widget>
cvaliere commented 8 years ago

to be sure I understand:

thanks

hirbod commented 8 years ago

As long as you want to make XHR request on file:///, you will need engine-localhost - otherwise, a lot of thing won't work. Also make sure, that you've pointed out correct policies inside of meta-data (default-src, img etc.)

hirbod commented 8 years ago

By the way: Just want to spit out an important warning here:

• From current iOS 10 Beta, I would recommend to stay FAR away from WKWebView and stick back to UIWebView, as user-scalable=no won't be supported. This is on apples radar, but I am damn sure that they won't listen to there users again. This means, that "PINCH TO ZOOM" will be active automatically and can't be prevented (maybe with some dirty touch-hacks, but I did not test this)

This warning comes also from famous Developer @EddyVerbruggen and some other Cordova-Team members

Edit: Source: https://github.com/Telerik-Verified-Plugins/WKWebView/issues/249

wf9a5m75 commented 8 years ago

currently, your master branch doesn't require to have WKWebView, right?

yes

but your new multiple_maps branch requires it, right?

yes, at this time.

is it the reason why we don't see the map on iOS?

probably.

WKWebView seems to add some shitty problems, particularly on iOS8, are you aware of any?

Oh, I see, but in order to test, please add it.

cvaliere commented 8 years ago

@Hirbod can you give the reference to the issue you're talking about?

hirbod commented 8 years ago

edited my post

cvaliere commented 8 years ago

seems fixed in iOS 10 beta 4? https://www.idevice.ro/2016/08/01/ios-10-beta-4/

cvaliere commented 8 years ago

we still can't manage to make it work :( now, with the wkwebview-engine-localhost, the map appears, the markers appear, but all others objects in the page are not responding to touch (menu, tabs, etc...)

do you have a working example on iOS?

wf9a5m75 commented 8 years ago

As I said, the map and the marker classes only. And I've been working on the iOS touch issue.

cvaliere commented 8 years ago

We are testing with only map and marker But the rest of the page (HTML) are not responding to touch What do you mean "I've been working on the iOS touch issue"?

elpsk commented 8 years ago

Hi @wf9a5m75 , here you can find the demo app that reproduce the touch issue on iOS. Please let me know what i'm doing wrong...

https://github.com/elpsk/cordova-plugin-googlemaps-multiplemaps-ios

thanks

wf9a5m75 commented 8 years ago

@cvaliere

What do you mean "I've been working on the iOS touch issue"?

I'm fixing this problem But the rest of the page (HTML) are not responding to touch.

@elpsk

Thank you, but I already know it and working to fix it.

cvaliere commented 8 years ago

@wf9a5m75 can you tell us when this plugin is able to display a map and a marker, please? there is no point that we lose our time trying to make it work, if it's not even able to correctly display a map...

wf9a5m75 commented 8 years ago

No ETA. Sorry.

cvaliere commented 8 years ago

dude, you remember what you wrote in March?

Hi everyone, this is Masashi who is the original author of this plugin. I have already known this issue, many people crime to me since old versions, and I know what I should do. I left this project at once (because it was crazy busy), but I came back to this project again. I will buckle down this issue also. Hold on please.

it's unbelievable some nice people were trying to make this plugin better, they all have stopped their work because you said you would take care of it, and 6 months after, it's not even able to display a map?? how disrespectful are you?

rparcus commented 8 years ago

Hey @cvaliere, just trowing my 2 cents out there.. Instead of having to use the optimized version which is not supported, did you try to handle the marker creation in a different way on javascript? I also have lots of markers in my app and things work just fine by throttling the creation and giving some time to the UI to refresh every few miliseconds..

wf9a5m75 commented 8 years ago

If you disappoint my work, sorry. But it's open source. You can folk and modify, create your version anytime. I said many times in many issue threads, my time is very limited every day.

it's not even able to display a map??

At least on my iOS simulater and iOS device, no problem to display the maps. But I know the touch issue on iOS, and working to fix it in couple of days.

cvaliere commented 8 years ago

@quiuquio thank you for the idea, but creating markers with this plugin is slow, period; it is a nightmare on Android, and a smaller nightmare on iOS, but still a nightmare; according to masashi's tests, the new branch is 3 times faster on iOS, and 30 times faster on Android(!!) And also, I need multiple maps

@wf9a5m75 your work is open source, but you killed every will of participating when you came back in march and said that you knew what you had to do and that you would do it. Since then, no one has done anything because we are all waiting for you to release what you promised! And 6 months and 143 commits later, it is still not even able to display a map, it's crazy

At least on my iOS simulater and iOS device, no problem to display the maps

Yes, strictly speaking, it is able to display a map; but in exchange, it destroys the rest of my application. Let me think if I wanna use it... errrrr... no

adrinavarro commented 8 years ago

We're also having significant issues regarding the extreme slowness & memory usage on Android. We'd be happy to sponsor any patches and related features such as clustering, however, it seems that there's no such possibility at this moment.

Masashi: Your plugin is great. It has enabled many of us to build good apps that include maps. It is stable, very versatile and well documented. I also understand you're very busy and have to deal with personal matters as well. Please, consider encouraging and including other developers' efforts so that we can have an even better plugin for the community to use.

I reckon that many of us use this plugin in commercial projects and time is always a factor when building commercial apps.

cvaliere commented 8 years ago

@wf9a5m75 I see that you commited 'Bug fix : The plugin detects user click position on iOS.' does it mean that it works now? thanks

wf9a5m75 commented 8 years ago

Finally (I believe so), the problem of clicking position is incorrect on iOS is fixed.

And one more thing, next version will allow you to use fixed position (such as header). This means the plugin watches all HTML element positions periodically instead of the child elements. This is implemented on iOS currently (Android is not implemented yet). https://youtu.be/ajysxJ9H6rc

cvaliere commented 8 years ago

great, gonna test that

cvaliere commented 8 years ago

doesn't work dunno why

* Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '* setObjectForKey: object cannot be nil (key: map_0_293554574284)' *\ First throw call stack: (0x27c8049f 0x35436c8b 0x27b9dc63 0x245687 0x20bcbb 0x16e09db 0x16e09c7 0x16e43ed 0x27c463b1 0x27c44ab1 0x27b923c1 0x27b921d3 0x2ef900a9 0x2b1a1fa1 0x3bb43 0x359b6aaf) libc++abi.dylib: terminating with uncaught exception of type NSException

wf9a5m75 commented 8 years ago

Please post all stack logs, not just first row.

wf9a5m75 commented 8 years ago

@cvaliere Retry with https://github.com/mapsplugin/cordova-plugin-googlemaps/tree/f4319e35c72740cca2ce71c4f2d9770b5da3efce

cvaliere commented 8 years ago

nope I lost enough time with your it's-been-6-months-but-I-still-don't-work plugin we found another solution

wf9a5m75 commented 8 years ago

After I posted the last comment, I already committed another bug fixes. But anyway, you found another solution, that's fine. Please go ahead. I will go on my way, on my pace.

adrinavarro commented 8 years ago

@cvaliere Would you mind sharing your solution? Thanks!!

cvaliere commented 8 years ago

@adrinavarro on Android, we use branch optimization of this plugin, which works well and on iOS, we continue, sadly, to use branch master, which main flaw is that it freezes UIWebView randomly when removing markers; after transiting to WKWebView with Ionic's brand new plugin (https://github.com/driftyco/cordova-plugin-wkwebview-engine), it doesn't freeze anymore I'd still love to use multiple_maps, but, hey... no ETA!

adrinavarro commented 8 years ago

Well, it makes me happy to hear that! We'll see if the optimization branch helps us (we have 500+ markers… we were hoping for clustering as well, right now anything is bad). I'm excited for WKWebView as well, we were wondering if it was production ready though.

hirbod commented 8 years ago

Wow that WKWebView Plugin just worked perfectly. The XHR interceptor is so simple and still so genius. Never thought about that. I changed it flawlessly inside of my projects, thanks for that solution.

cvaliere commented 8 years ago

well, Ionic released this plugin last week, and they say it's not production ready yet; we made our own tests, and it seems OK for us, a lot better than the official apache plugin or the one from Telerik 500+ markers is a lot; shouldn't be too big a problem with branch optimization for Android and WKWebView for iOS, but have you considered making clustering server-side?

atannus commented 8 years ago

Clustering is a user-space decision, it should not be a plugin feature.

A cluster is simply a marker that represents multiple places/things. The user should handle clustering and insert a single marker where the cluster is. It's a lot simpler and flexible to think of it this way.

Having the server-side return the positions and sizes of clusters follows this general guideline. The plugin should concentrate on exposing the Android Maps API through Cordova, not reinventing it.

Please see my Feb 3 comment.