search

code-for-chapel-hill / NC-COVID-Support

Community support site, supporting COVID-19 business opening hours, Food Banks, School Meals, Farms and Social Services.
https://nccovidsupport.org
GNU General Public License v3.0
16 stars 39 forks source link

Add marker name labels to the businesses #147

Open readingdancer opened 4 years ago

readingdancer commented 4 years ago

Have an idea for an additional feature? (a) Describe the feature

In the same way that Google Maps shows labels if you search for a Restaurant, we also would like to display the names above ( or beside ) the markers. I think this functionality already exists in LeafletJS.

(b) How does it need to perform?

As per Google, if they can all fit, show them, if they over lap, don't :)

(c) How will we code it?

Hopefully LeafetJS will do this for us with a relatively small amount of code.

readingdancer commented 4 years ago

@dsummersl - If you have any spare time, would you like to tackle this ticket as it's related to the markers, so you know that section of code well :) ?

dsummersl commented 4 years ago

Sure, I should have time this week.

On Apr 26, 2020, at 9:26 PM, Chris Houston notifications@github.com wrote:

@dsummersl https://github.com/dsummersl - If you have any spare time, would you like to tackle this ticket as it's related to the markers, so you know that section of code well :) ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/code-for-chapel-hill/NC-COVID-Support/issues/147#issuecomment-619660744, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAHRCHUBSWLWJP5CD2PG3DROTNLVANCNFSM4MLRPI5A.

readingdancer commented 4 years ago

@dsummersl - I started to have a look at this today as it crossed over with another ticket I was looking at. I implemented it by forking the Leaflet.ExtraMarkers project. You can see the branch here: https://github.com/readingdancer/Leaflet.ExtraMarkers/tree/Add-Names

That branch allows you to add a Name option which we can then position using CSS.

However, at the moment, I don't like how things look where there are a lot of close markers, so we need to decided how we are going to handle this situation.

See the below screen grab.

@KM-Hanson - Tagging you as well so you can give your thoughts.

Place Names

The master branch ( will soon ) have this new code inside it, but with the two lines commented out for adding the names. Obviously if we need to do more to change the Extra Markers plugin, it would be good to send them back a PR, for now I removed the dependency and added the distribution files directly to our solution.

dsummersl commented 4 years ago

That's tough! Looking at the masters, google maps, I see they're maybe more clever than we can be: they don't show all labels if they would overlap a pin or other label.

image

I don't know the leaflet API well enough to know how plausible something similar would be...

Another option maybe would be the 'leaflet-tooltip-layout' library which would scoot labels far enough away to prevent any overlaps...I don't really like the way its default styles look (or distance from pins), but maybe we could restyle it enough to get a look we'd like:

https://zijingpeng.github.io/overlapping-avoided-tooltip/

coryasilva commented 4 years ago

It seems like Google maps is using some sort of rules based labeling engine. It seems to have rules for not only intersecting with other labels & markers but some basemap icons and labels.

Though I think you can get away with a simple rules engine. It seems that they are only choosing between three possibilities:

  1. Left
  2. Right
  3. None

Maybe the rules look something like this: Calculate label polygon p

All in all this rudimentary logic might be enough and stave off a full k-d tree implementation...

readingdancer commented 4 years ago

@coryasilva thanks for your reply, I didn't see it until now.

Have you any idea how you calculate the bits you mentioned above? I agree, that would certainly be a good improvement over not having any labels at all.

jasonajones73 commented 4 years ago

@readingdancer Instead of having a permanent label, is there any reason the simple mouseover tooltip that Leaflet offers wouldn't be acceptable?

I could probably get that added if that would work for now.

readingdancer commented 4 years ago

@jasonajones73 - Yes, we want permenant labels so they are visible without having to click on them so people can see the business at a glance. in the same way you can on Google Maps etc. If you have 50 pins on the map, you really don't want to have to click on them all to find the right one :)

jasonajones73 commented 4 years ago

@readingdancer that makes sense. Tooltips only show up on mouseover so no click is required but I understand what you are saying. The effort is approximately the same. I just wanted to check since I didn't see it in the thread. I was looking through some Stack Overflow Q&A and noticed that most people resolved this by implementing permanent tooltips and applying their own CSS styling. That doesn't really fix label overlap though. I was playing around with the project (while trying to learn more about Vue) and actually have the permanent tooltips working. If pursuing this is of interest, I'm happy to take this on as something that could get scrapped if it doesn't end up looking great or working out. I'm wondering also if there might be some thoughts on bare-bones logic to filter the labels that are shown. Maybe something like, only showing markers for businesses that are open?

readingdancer commented 4 years ago

Hi Jason,

Currently we are showing all markers for businesses that are open ( at least one day per week ) and the icons are shown in grey if the they are closed "today" or.. if you select a day in the drop down, then it's indicating if the business is closed on that day of the week.

I've looked into what you are suggesting as well, but I found in some areas when the markers are close together it just becomes unreadable. It needs some kind of logic to position the labels left / top / right depending on whatever else is around it ( I think ).

Happy for you to have a go at cracking this nut :)

Cheers, Chris

readingdancer commented 4 years ago

This is still up for grabs if anyone would like a challenge 😄

jasonajones73 commented 4 years ago

Sorry I was never able to offer a solution for this @readingdancer. Despite quite a bit of searching on Stack Overflow and browsing through Leaflet documentation and plugins, I was never able to come up with anything other than the permanent tooltips I mentioned above. They don't "intelligently display" though. Sorry about that! I really want to know what the solution to this is if someone finds it.

fredlawl commented 4 years ago

@readingdancer I can take this over.

fredlawl commented 4 years ago

Okay so, big update here. I decided I have to put this on pause. This kind of turned into a huge time sink for me. Here's what I discovered:

So in theory, this task isn't too terrible:

  1. Get all icons that are in view
  2. Create labels in DOM, hide, and cache them in a lookup table
  3. Perform collision detection on Zoom
  4. Draw labels next to their icons, and they move around as the map moves

Before I began, I created two commits to help support the rest that comes:

  1. 60d166 ("ResourceMap: Replace inline zoom function with method")
  2. c61f08 ("ResourceMap: Make map determine what markers are in view")

Next, I went through multiple iterations, and I landed in two commits:

  1. https://github.com/code-for-chapel-hill/NC-COVID-Support/commit/457ce580fc62dad35aadccbc12797a52a45187f0 (earliest)
  2. https://github.com/code-for-chapel-hill/NC-COVID-Support/compare/master...fredlawl:issue-147 (latest)

I'll be referring to these two commits going forward as latest, and earliest respectively.

In both commits, I used a rectangular collision detection algorithm. I thought this was best because, even though polynomial detection is better, ie, give the "hitbox" take up less space, it just wasn't worth it to dive deeper for this project. So the hitbox is a "big" rectangle that encapsulates the marker width + height + the width + height of the title text CSS. A big part in figuring this out (without having to mess with calculating the size of these things with varying fonts) was to draw the labels first, cache, and then based off how they're drawn, I have the dimensions to create this magical hitbox.

With that said, both commits have a specific common problem with them: the leaflet plugin (more on this below)

The earliest commit demonstrates some of the performance areas I was trying out: using CSS to do the drawing for the most part, caching, and doing specific things on specific events. The problem with this approach is that it's a mess, and is slightly not as great performance wise because of that relying on DOM to toggle CSS classes. This approach used a more complicated version of rectangle collision. Trivial issues to resolve, but they're there and havn't been addressed in full. The big difference between this implementation and the latest implementation is that the collision handles more cases, but as we will find out with the latest implementation, that's not that big of a deal.

The latest commit is more interesting. Because I was running into the issue of slow testing, I decided I wanted a faster way to iterate to verify the algorithm works. (turns out what I had was basically working all along--even with the earliest commit, but there's that afore mentioned problem...) So I created a test program: https://github.com/fredlawl/Toy-Map-Marker-Collision-Detection (C++, compiles trivially in Visual Studio). This allowed me to come up with a more simpler algorithm, and to think of a way to model this implementation. Soon, I implemented this implementation in the latest commit. Now, the difference between the latest commit and the earliest this time was the focus on algorithm correctness. Therefore, all the before optimizations were thrown out the window to just get something working. This latest commit also has the problem of not "hiding" labels at different zoom levels anymore. This can be easily fixed, but this where I stopped.

I noticed that the common problem between both solutions was that the text boxes were STILL overlapping with adjacent markers in close proximity, or would not draw then they potentially could. It wasn't until after I went down this long rabbithole before I figured out that the library that does the marker clustering, adds markers to the view AFTER my algorithm has ran (or even during...async events and whatnot), so what was happening was that the collisions weren't actually being accounted for correctly. This is my theory of where this is all going wrong because my test C++ program that models the latest commit, actually works quite well. Not perfect, but well enough for this project.

In conclusion, this is where I gotta drop this. Early on, I ran into a lot of trouble with the leaflet library, and it just takes way too long to fully read the documentation, understand how markers are being placed, and what events are fired in what order to take this further. As an example, I spent way too much of a Saturday just to figure out what markers were actually drawn on the screen; maybe I was naive and missed something of the library, or it was really just that hard to work with.

Hopefully with these findings, someone is able to take this to the next step, and I'm willing to offer assistance to anyone who wants to pick this up. I just can't find the time to tackle this with some of my other responsibilities.

Lastly, there could be some additional improvements to this design: eg. use canvas for extra performance on drawing, or other more optimal CSS tricks, but I've found that even with what I have, it actually performs quite well (other than the marker race condition)

EDIT @readingdancer I'd be more than happy to open up a separate PR on two pre-commits I made for this feature. They move the responsibilities of the components around a little bit, and offer a better design in that regard. IMHO, so let me know if you want me to open up a PR for them.

coryasilva commented 4 years ago

This project might be of interest for you guys. (Unfortunately, I have not had much time for personal programming projects...) https://github.com/Geovation/labelgun

fredlawl commented 4 years ago

This project might be of interest for you guys. (Unfortunately, I have not had much time for personal programming projects...) https://github.com/Geovation/labelgun

Welp, I feel bad now heh. Oh well... half the fun is the learning, right?

readingdancer commented 4 years ago

Definitely don't feel bad about it @fredlawl I think what you have built ( if we can fix the plugin issue ) is actually better than this Label Gun example, if you look at the demo, the labels might not be clashing with each other, but they do go over the top of other pins, where as your demo you gave us, this is also avoided.

I am going to try and take a look at this over the next week when I get a bit of distraction free time :)

readingdancer commented 4 years ago

@coryasilva - Thank you for the link, it might also be useful for us, so I will be taking a look as well :)

fredlawl commented 4 years ago

Definitely don't feel bad about it @fredlawl I think what you have built ( if we can fix the plugin issue ) is actually better than this Label Gun example, if you look at the demo, the labels might not be clashing with each other, but they do go over the top of other pins, where as your demo you gave us, this is also avoided.

I am going to try and take a look at this over the next week when I get a bit of distraction free time :)

What did you think about the two pre-commits I put together? I think those could be valuable separating the responsibility of that from the parent component to the map component.

coryasilva commented 4 years ago

@fredlawl Yes, nearly all the fun is in learning! I hope labelgun informs the final solution -- I did not mean it for a drop in replacement for what you are doing.