Change some amenity colors to grey

[Adamant36]

This PR changes the color of some amenities to grey. The purpose of it is to "move some culturally insignificant, man made objects to man-made-grey, in order to offload the semantics in the amenity-brown category." Closes #3326

Bench

Picnic table

Waste basket

Waste disposal

Recycling

Shelter

[matthijsmelissen]

Just to remind me, what was the idea or philosophy behind this?

[Adamant36]

Did you look in the original issue? Im pretty sure the reason is given there.

[kocio-pl]

Please add short summary/rationale for every PR, so it's self-contained. If there were some conclusions, they should be clear without reading all the discussions - they are usually complicated.

[Adamant36]

@kocio-pl, my bad. Its hard to write a rational for issues where I wasnt involved in the creation of it or the disscussion at all. Plus, sometimes people's reasons for wanting things a certain way go over my head. But I'll do a better job at it in the future anyway.

[sommerluk]

I wasnt involved in the creation or disscussion at all

That’s something I don’t understand. Because this PR is created by you. And you also have participated in the discussion of #3326 (the issue that is closed by this PR) since the very beginning.

[kocio-pl]

I'm happy that you're making PRs and learning details.

When you create PR, you are backing this proposition, so it might be from your point of view, but should be pretty clear why it's a good change and why it's needed. For example:

• why these objects and not some others?
• why this color and not something else?

[polarbearing]

IIRC the idea was to move some culturally insignificant, man made objects to man-made-grey, in order to offload the semantics in the amenity-brown category.

[Tomasz-W]

@Adamant36 I noticed that you are choosing isolated places for test renderings, but you should pick them rather opposite - it's important to know how certain change would match with another map elements and how it looks on different backgrounds (eg. grey amenity=shelter on landuse=forest but also on highway=platform area)
PS. Please contact me via e-mail provided in my profile :)

[Tomasz-W]

@Adamant36 Can you left brown icon for amenity=recycling + recycling_type=centre combination?

[kocio-pl]

Related to #3395.

[sommerluk]

Yes, more test rendering – maybe at different zoom levels – would be great. You need to show that the PR you propose actually works. So we can discuss it. (Don’t worry, I think that for this PR, chances are quite good.)

[Adamant36]

@Tomasz-W and @sommerluk, I'll add more test renderings. Kocio-pl told me several times I need to crop things more to make the icons easier to spot. I guess I over croped in this situation though. I'll fix the recycling thing also.

[Adamant36]

More tests

Benches



Recycling containers



Picnic tables





Shelters


Waste Baskets

Uncle Billy's Cabin

@Tomasz-W, I fixed the recycling thing. Its back to brown.

[Tomasz-W]

@Adamant36 amenity=recycling node without recycling_type=* tag is like a container by default, so it should be in man-made-grey. Is it considered in the code? (sorry, but as I'm not a coder I can't check this indyvidually)

[Adamant36]

I was thinking about that and I couldnt decide. So I just left it amenity brown. To me its neutral since we dont know if its a centre or a bin. As it is, the wiki doesnt sound like it has a default. Im fine with grey anyways though.

[polarbearing]

Thanks for the tests. As for the cropping, it probably depends on the stage of the proposal. When introducing an idea, it's good to focus on the new detail, while at this stage it's important to see it in context.

In favour of the change. I wonder, the college example has both brown and grey recycling, but I find it unlikely to have recycling centres in these places? Maybe a tagging error?

For naming the category, I'd like something like infrastructure_minor or so?

[Adamant36]

@polarbearing, thanks for the details. That makes sense.

As far as the college example goes, its probably the difference between the type of recycling it is being tagged or not. I was talking about it above. It kind of presents a problem if we have containers tagged as gray, but then recycling centers and no category tagged as brown, because inevitably some of the places without the type tag will be containers, but still show up as brown when they shouldn't. Maybe a solution would be another third color for amenity=recycling. Since we technically don't know if it is an amenity or not. It would be interesting to know how many other tags out there are that change if a modifier can be added to them. Maybe an "unknown" color category could be created so mappers can know if something needs be tagged with a type tag. Or we could just leave recycling out of the grey all together.

[Tomasz-W]

Before discussion in this ticket I didn't even know about recycling_type=* key, and I was marking recycling containers just by amenity=recycling. I think a lot of people map them like that, so I suggest to render amenity=recycling in man-made-grey. It's a lot more possible that someone forgot to add recycling_type=* key to simple container than to much bigger recycling centre,

[kocio-pl]

Please be more precise - we have 3 main cases, which should be brown, gray (or anything else)?:

• amenity=recycling
• amenity=recycling + recycling_type=container
• amenity=recycling + recycling_type=centre (it's rendered earlier)

[Tomasz-W]

Sure, I mean:

• amenity=recycling -> grey
• amenity=recycling + recycling_type=container ->grey
• amenity=recycling + recycling_type=centre -> brown

[polarbearing]

Yes the unknown one should be treated like the minor infrastructure case, i.e. the container.
Only if the major type (centre) is specified, the amenity colour can be awarded.

[Adamant36]

@Tomasz-W and @polarbearing, makes sense. I'll update it when I get a chance.

[Adamant36]

Fixed it. It should be good to go now.

recycling center in brown

regular recycling in gray (ignore the name)

[kocio-pl]

It looks quite readable, but it means that [feature = 'amenity_recycling'][zoom >= 19] covers also recycling centers on z19+. It might accidentally work because of the order of rules Carto CSS is applying, but it would be harder to debug problems. I think instead of these two lines we should just use something like [feature = 'amenity_recycling'][recycling_type != 'centre'][zoom >= 19].

[Adamant36]

@kocio-pl, I updated the code. So, it should be good to go now. Here's some tests with the new code.
Normal recycling in grey

recycling center in brown

recycling container in grey

[Adamant36]

@kocio-pl, I updated it and I think the changes you wanted are fixed now. Also, I tested it on my side and it seemed to work. Although, I didn't do any new test rendering shots.

[Adamant36]

@kocio-pl, I think its good to go now.
Normal recycling grey

recycling container grey

recycling center brown

[kocio-pl]

It also looks like indentation issue.

[Adamant36]

@kocio-pl, sorry about that. Hopefully its fixed now (I'm keeping my fingers crossed).

[kocio-pl]

I have a problem with this code. It has some easy parts, when some lines are replaced, but also some blocks are moved in some other place etc. and I have a hard time following what is really done this way. For example this object has lost its label instead of just changing the color:

My recommendation is to make it as simple as possible and not clean code along the way (which I suspect is what you've tried to do) to avoid possibility of introducing bugs. Cleaning is always possible in separate PRs and the files are long enough to get lost easily.

[Adamant36]

@kocio-pl, I didn't clean any code along the way. Your the one that wanted the recycling bit simplified. That might be whats causing the problem. Since it seemed to work fine before that. There was also three conflict that needed resolving due to it being a month old now. Its probably related to that somehow. Since the recycling part seems to be whats causing all the issues, can I just take it out and do it in a separate PR? I rather just get the simple, none problematic things dealt with, and deal with it on its own.

This is one of the reasons I'm sick of Issues involving multiple issues. I see no reason why things can't be split up more. Especially if its going to hold things up for a month by involving extra testing/fixing that wouldn't be necessary otherwise.

[kocio-pl]

Sorry for the time it took. I think you're right - in this case it's best if you will make the split and I will check it one by one soon.

However sometimes it's better to make the one big PR, for example it worked goof for adding and fixing towers. I'm not sure what is the golden rule when to split things up or not to do it...

[polarbearing]

Do I understand correctly that it's just the PR being closed bu #3326 still on the agenda?

[Adamant36]

@polarbearing. Yes, I closed it to split it up into a couple of PRs. The branch was behind by like 10 commits and something was screwing up with the recycling part in the proccess of resolving conflicts that I couldnt figure out. So im going to do it separate from the other stuff with an updated branch.