Render most shops on z17 as dots only #2589

Merged
merged 1 commit into from Apr 19, 2017

Conversation

Projects
None yet
5 participants
@matthijsmelissen
Collaborator

matthijsmelissen commented Mar 22, 2017

This cleans up the overcrowded z17 significantly:

Before:
screenshot from 2017-03-22 23-42-38

After:
screenshot from 2017-03-22 23-41-18

@matthijsmelissen

This comment has been minimized.

Show comment
Hide comment
@matthijsmelissen

matthijsmelissen Mar 22, 2017

Collaborator

This PR introduces quite a lot of duplicate code, can anybody think of a more elegant way to accomplish this?

Collaborator

matthijsmelissen commented Mar 22, 2017

This PR introduces quite a lot of duplicate code, can anybody think of a more elegant way to accomplish this?

@kocio-pl

This comment has been minimized.

Show comment
Hide comment
@kocio-pl

kocio-pl Mar 22, 2017

Collaborator

This would resolve at least part of #2444.

Collaborator

kocio-pl commented Mar 22, 2017

This would resolve at least part of #2444.

@kocio-pl

This comment has been minimized.

Show comment
Hide comment
@kocio-pl

kocio-pl Mar 23, 2017

Collaborator

This PR can also help resolve #1560.

Collaborator

kocio-pl commented Mar 23, 2017

This PR can also help resolve #1560.

@nebulon42

This comment has been minimized.

Show comment
Hide comment
@nebulon42

nebulon42 Mar 23, 2017

Contributor

I'm not sure why I would need to know that there is some sort of shop (= dot without name). Why not leave it out completely and display it later with icon?

Contributor

nebulon42 commented Mar 23, 2017

I'm not sure why I would need to know that there is some sort of shop (= dot without name). Why not leave it out completely and display it later with icon?

amenity-points.mss
- [feature = 'shop_art'][zoom >= 17] {
- marker-file: url('symbols/shop/art.svg');
+ [feature = 'shop_art'][zoom >= 18] {
+ [zoom < 18] {

This comment has been minimized.

@nebulon42

nebulon42 Mar 23, 2017

Contributor

This can never be rendered or am I missing something?

@nebulon42

nebulon42 Mar 23, 2017

Contributor

This can never be rendered or am I missing something?

amenity-points.mss
- marker-file: url('symbols/shop/bakery.svg');
+ [feature = 'shop_bakery'][zoom >= 18] {
+ [zoom < 18] {
+ marker-width: 4;

This comment has been minimized.

@nebulon42

nebulon42 Mar 23, 2017

Contributor

I think this misses marker-line-width: 0 to avoid the darker outlines that can be seen in the screenshot.

@nebulon42

nebulon42 Mar 23, 2017

Contributor

I think this misses marker-line-width: 0 to avoid the darker outlines that can be seen in the screenshot.

amenity-points.mss
@@ -567,7 +567,12 @@
[feature = 'shop_other'][zoom >= 17] {
marker-fill: @shop-icon;
- marker-width: 6;
+ [zoom < 18] {

This comment has been minimized.

@nebulon42

nebulon42 Mar 23, 2017

Contributor

Why not try to place all generic dots here to avoid duplication for each feature? At least the filter has to be duplicated though.

@nebulon42

nebulon42 Mar 23, 2017

Contributor

Why not try to place all generic dots here to avoid duplication for each feature? At least the filter has to be duplicated though.

@nebulon42

This comment has been minimized.

Show comment
Hide comment
@nebulon42

nebulon42 Mar 23, 2017

Contributor

I was thinking of a rewrite of the POI logic to combine the icons and texts into a shield like for place names. With that we could get rid of names without icon and the text-dy mess. We could also try to place the text above the icon if there is not enough space below. I wanted to do a first pass with shields and a second pass with icons only.

Maybe this change here would go well with that rewrite?

Contributor

nebulon42 commented Mar 23, 2017

I was thinking of a rewrite of the POI logic to combine the icons and texts into a shield like for place names. With that we could get rid of names without icon and the text-dy mess. We could also try to place the text above the icon if there is not enough space below. I wanted to do a first pass with shields and a second pass with icons only.

Maybe this change here would go well with that rewrite?

@kocio-pl

This comment has been minimized.

Show comment
Hide comment
@kocio-pl

kocio-pl Mar 23, 2017

Collaborator

I'm not sure why I would need to know that there is some sort of shop (= dot without name). Why not leave it out completely and display it later with icon?

This was the only proposition which had any kind of support and nobody complained when I wanted to implement it: #2444 (comment).

Collaborator

kocio-pl commented Mar 23, 2017

I'm not sure why I would need to know that there is some sort of shop (= dot without name). Why not leave it out completely and display it later with icon?

This was the only proposition which had any kind of support and nobody complained when I wanted to implement it: #2444 (comment).

@dieterdreist

This comment has been minimized.

Show comment
Hide comment
@dieterdreist

dieterdreist Mar 23, 2017

@matthijsmelissen

This comment has been minimized.

Show comment
Hide comment
@matthijsmelissen

matthijsmelissen Mar 23, 2017

Collaborator

I'm not sure why I would need to know that there is some sort of shop (= dot without name).

In areas that are not so dense (like rural areas or suburbs), it might be useful to know where the shops are in the village.

Collaborator

matthijsmelissen commented Mar 23, 2017

I'm not sure why I would need to know that there is some sort of shop (= dot without name).

In areas that are not so dense (like rural areas or suburbs), it might be useful to know where the shops are in the village.

@nebulon42

This comment has been minimized.

Show comment
Hide comment
@nebulon42

nebulon42 Mar 23, 2017

Contributor

Ok I understand. It is an improvement to the status quo.

Contributor

nebulon42 commented Mar 23, 2017

Ok I understand. It is an improvement to the status quo.

@nebulon42

This comment has been minimized.

Show comment
Hide comment
Contributor

nebulon42 commented Mar 24, 2017

@matthijsmelissen

This comment has been minimized.

Show comment
Hide comment
@matthijsmelissen

matthijsmelissen Apr 11, 2017

Collaborator

I provided another solution (hope you don't mind, @nebulon42). This solution saves a lot of code, while it should still avoid the combinatorial explosion.

Could somebody review the code?

Collaborator

matthijsmelissen commented Apr 11, 2017

I provided another solution (hope you don't mind, @nebulon42). This solution saves a lot of code, while it should still avoid the combinatorial explosion.

Could somebody review the code?

@matthijsmelissen

This comment has been minimized.

Show comment
Hide comment
@matthijsmelissen

matthijsmelissen Apr 11, 2017

Collaborator

z17 looks now as follows:

screenshot_2017-04-12_00-28-26

Collaborator

matthijsmelissen commented Apr 11, 2017

z17 looks now as follows:

screenshot_2017-04-12_00-28-26

@matthijsmelissen

This comment has been minimized.

Show comment
Hide comment
@matthijsmelissen

matthijsmelissen Apr 11, 2017

Collaborator

On closer look, this seems to combine well with @nebulon42's solution, so we can either merge his proposal first and I will adapt, or the other way around.

Collaborator

matthijsmelissen commented Apr 11, 2017

On closer look, this seems to combine well with @nebulon42's solution, so we can either merge his proposal first and I will adapt, or the other way around.

@HolgerJeromin HolgerJeromin referenced this pull request Apr 13, 2017

Closed

Release v3.2.0 #2605

@matthijsmelissen

This comment has been minimized.

Show comment
Hide comment
@matthijsmelissen

matthijsmelissen Apr 14, 2017

Collaborator

Could somebody have a quick look at the new code?

Collaborator

matthijsmelissen commented Apr 14, 2017

Could somebody have a quick look at the new code?

@pnorman

I haven't run it, but the code looks good to me. I have no opinion on merge order.

The preview shows two types of dots

image

image

These should either be the same, or clearly different with an obvious meaning.

@matthijsmelissen matthijsmelissen dismissed stale reviews from nebulon42 and pnorman Apr 19, 2017

Addressed issues.

@matthijsmelissen matthijsmelissen merged commit 85b8f26 into gravitystorm:master Apr 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matthijsmelissen matthijsmelissen deleted the matthijsmelissen:shops-as-dots branch Sep 14, 2017

@matthijsmelissen matthijsmelissen restored the matthijsmelissen:shops-as-dots branch Nov 11, 2017

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