Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

Finish maki icons #127

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Finish maki icons #127

wants to merge 7 commits into from

Conversation

v1r0x
Copy link
Contributor

@v1r0x v1r0x commented Feb 5, 2016

This PR fixes #120

  • Throttle API calls
  • Stop adding poi icons if the layer has been disabled (couldn't find a solution, any idea? @Henni)
  • Add missing maki icons to zoom levels (10-15)
  • Move marker from lvl 9-15 to 11-16 (or even 12-16)
  • Fix zoom/move performance, maybe with marker clustering
  • Fix POI load performance
  • Overall performance

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 5, 2016

cc @Henni @jancborchardt

@Henni
Copy link
Contributor

Henni commented Feb 5, 2016

At first glance it looks like you just delay all requests. But I haven't tested it in detail yet.

Maybe take a look at underscore.js' implementation
(I believe you are allowed to copy that code as it's MIT licensed. Maybe just add a comment to reference underscore.js)

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 5, 2016

Maybe take a look at underscore.js' implementation
(I believe you are allowed to copy that code as it's MIT licensed. Maybe just add a comment to reference underscore.js)

I'm not sure if it is possible to use this, since the function has different parameters on every call. If you have an idea or want to test it, just do it! ;)
JS' async stuff really bugs me, so I'd be very happy if you'd have a solution for the API calls and the second TODO in the OP.

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 6, 2016

I modified the code. Worked for me in some first tests on my test machine. So it could work on all machines ;)
Would be great if you could test it, after your exams. @Henni

@Henni
Copy link
Contributor

Henni commented Feb 10, 2016

Still throws:
image

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 10, 2016

Bummer...maybe we have to increase the timeout. Could you test int with 200 or 250ms in this line?

@Henni
Copy link
Contributor

Henni commented Feb 15, 2016

A lot of these errors:
image

But no 429s anymore.

Moving around freezes the app (sometimes even the browser). I'm not sure if this PR is responsible for this performance issue.

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 15, 2016

I think this is related to the PR/branch. On lower zoom levels (~ <10) there are a lot of icons and layers which have to be rendered. It also loads a lot of icons in the background (which should be async). Don't know what's the best way to fix this.

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 24, 2016

Maybe we could use something like that?
But still we have to fix the async load performance.

I'll add some TODOs to the OP.

@Henni
Copy link
Contributor

Henni commented Feb 25, 2016

MarkerClusters would fix the visual glitchiness, but the load performance still remains as you said.

@v1r0x Take a look at Web Workers. If it is possible to move the API calls to a worker this might solve the performance issue.

@v1r0x
Copy link
Contributor Author

v1r0x commented Feb 25, 2016

I added marker clustering and adjusted the zoom levels (from 9-16 to 12-16) and the performance is better. Not good, but better.
Remaining problem is the slow API for poi polling. The default (http://overpass-api.de/api/) is extremely slow, but the alternative (http://overpass.osm.rambler.ru/) is blocked by firefox.

@v1r0x
Copy link
Contributor Author

v1r0x commented Mar 9, 2016

Maybe we could cache the POIs in the database and check on startup (or somethink like once a week) for updates. Thus the load performance would be a lot better.

@v1r0x
Copy link
Contributor Author

v1r0x commented Apr 6, 2016

What do you think? @jancborchardt @Henni

@jancborchardt
Copy link
Contributor

@v1r0x maybe let’s merge this and fix the other stuff going forward?

@v1r0x
Copy link
Contributor Author

v1r0x commented Apr 6, 2016

If you are happy with the current state ;)

@jancborchardt
Copy link
Contributor

I’m not sure what these numbers are which suddenly appeared on the map:
capture du 2016-04-06 16-07-06
Almost seems like placeholders for icons to be loaded? There should just be nothing until loaded I’d say.

@v1r0x
Copy link
Contributor Author

v1r0x commented Apr 6, 2016

If there are several icons in the same area the get clustered so the app doesn't display thousands of icons and the UI starts to lag. If you zoom in they get separated (and clustered if you zoom out). Added this to fix #129. I thought I added the POI circle background, but seems I forgot that, maybe this way it doesn't look like a placeholder and can be associated with POIs?

@jancborchardt
Copy link
Contributor

Yup, the circle should be around it.

Also, do all POIs get clustered? I would say it only makes sense to group POIs which have the same type (like supermarket).

@v1r0x
Copy link
Contributor Author

v1r0x commented Apr 6, 2016

Yes, all together. Don't know if it is better if we only cluster POIs of the same type. Thus we would have a couple of cluster markers in one area and the user don't know which cluster is which POI type. Maybe I we'd add unique colors for every type and set this color for the specific cluster as well?

@jancborchardt
Copy link
Contributor

Maybe just use the normal type icon with an overlaid number on top? Or in a circle in a corner.

@v1r0x
Copy link
Contributor Author

v1r0x commented Apr 19, 2016

I hope I have some time this week to fix this. Would be good if we could finish the 0.1 release...finally ;)

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

Successfully merging this pull request may close these issues.

Finish maki icons (add more POIs/icons and throttle api calls)
3 participants