Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added historic aircraft rendering #4220

Closed
wants to merge 3 commits into from

Conversation

alexriabtsev
Copy link

Fixes #3518 (partly)

Changes proposed in this pull request:

  • added rendering for historic_aircraft

Test rendering with links to the example places:

Before

After

@kocio-pl
Copy link
Collaborator

I'm happy to see you making the code and I'd like to test it, but there are some things to clean before that:

  • first of all, the code involves historic=airfield (with just a few such objects...), not historic=aircraft :-)
  • I would rename the icon to aircraft.svg for sure, we don't have to stick with original name, it should fit the code of OSM Carto
  • there is unused airfield-15.svg, it should be just removed
  • the before/after images are missing
  • there are added spaces after few lines with CASE WHEN tunnel IN ('yes', 'culvert'), which should not be touched at all
  • z16 seems way too early to show single aircrafts, for such a small objects z18 should work much better

@alexriabtsev
Copy link
Author

I didn't get BEFORE/AFTER image but fixed everything else

@jeisenbe
Copy link
Collaborator

This PR would add a brown icon shaped like a propellor plane, seen from above.

I’m concerned that this could be confused with an airstrip, airfield or aerodrome - commonly these have a similar icon. Most of our icons which represent man-made objects are shown from the side.

Also, adding this feature would suggest that we might also need to render similar icons for other historic= objects which are about as common, such as historic=wreck, historic=cannon, historic=locomotive (See https://wiki.openstreetmap.org/wiki/Key:historic). Later this could lead to adding historic=tank, historic=ship, historic=vehicle and others, if we do not have a clear criteria.

See the open issues #3635 “Too many icons” and #3880 "Symbol and label prioritization” for the general discussion that needs consensus.

@jeisenbe jeisenbe added new features Requests to render new features POI labels Oct 15, 2020
@HolgerJeromin
Copy link
Contributor

Just to have the icon in the comments:
image

@alexriabtsev
Copy link
Author

This PR would add a brown icon shaped like a propellor plane, seen from above.

I’m concerned that this could be confused with an airstrip, airfield or aerodrome - commonly these have a similar icon. Most of our icons which represent man-made objects are shown from the side.

this icon was proposed in the original discussion. There is no problem to change svg in the folder when the community agrees on the different one.

Also, adding this feature would suggest that we might also need to render similar icons for other historic= objects which are about as common, such as historic=wreck, historic=cannon, historic=locomotive (See https://wiki.openstreetmap.org/wiki/Key:historic). Later this could lead to adding historic=tank, historic=ship, historic=vehicle and others, if we do not have a clear criteria.

the idea was/is to visualize one of the most wide-spread tag (see original issue). And yes, I would agree that this shouldn't be the only historic tag to be visualized.

@alexriabtsev
Copy link
Author

@kocio-pl if you don't mind add 'hacktoberfest-accepted' label for PR when accepted, please.

@kocio-pl kocio-pl added the hacktoberfest-accepted tag for valid PRs needed for https://hacktoberfest.digitalocean.com label Oct 15, 2020
@kocio-pl
Copy link
Collaborator

Since we use completely different color for transportation objects, I don't see how this could be misinterpreted as airstrip or airfield (let alone aerodrome, which has different, more contemporary shape). This shape is OK for me, since looks more "historic" than aerodrome and it fits our guidelines, but I see no problem if someone proposes replacement.

One missing thing to fix however should be to rescale it to 14x14 px (it looks in the code it's still 15 px square). Another question is what problem do you have with making images, do you need some help with it?

I don't get what is this tag for? I probably misunderstood you and added it now, so let me know, please, so I don't flip it back and forth...

@alexriabtsev
Copy link
Author

Since we use completely different color for transportation objects, I don't see how this could be misinterpreted as airstrip or airfield (let alone aerodrome, which has different, more contemporary shape). This shape is OK for me, since looks more "historic" than aerodrome and it fits our guidelines, but I see no problem if someone proposes replacement.

One missing thing to fix however should be to rescale it to 14x14 px (it looks in the code it's still 15 px square). Another question is what problem do you have with making images, do you need some help with it?

I don't get what is this tag for? I probably misunderstood you and added it now, so let me know, please, so I don't flip it back and forth...

thanks for your reply @kocio-pl! I would appreciate help with images as I'm not designer guy at all 👍 I can replace the image if I get the correct one.

Hacktoberfest is every year movement in open-source community. You can read more at https://hacktoberfest.digitalocean.com/

@kocio-pl
Copy link
Collaborator

They say about "valid pull requests", so I guess the tag is OK.

I'm not sure if I understand you, but this is not about graphics, but rather basic testing. Are you able to open Docker containers and view the proper area in Kosmtik? Then a simple screenshot should work. Rescaling icon might be probably easily done in Inkscape, but I guess editing SVG by hand might work too.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting a pull request. There are some technical issues with it - as pointed out by @kocio-pl but this is not the main problem.

I see the following issues currently speaking against accepting this here. Some of them have already been mentioned by @jeisenbe, many of them also reflect general problems mentioned previously in the discussion in #3635. Specifically:

  • the symbol you chose is not an intuitive representation for what it is supposed to represent, namely a non-functional aircraft. Accordingly there is a high risk of map users mistaking the rendering for representing something related to functional aircraft. The choice of color for the symbol does not solve this because the color systematics for symbols in this style are not very intuitive either.
  • no overall concept of common base design for symbology of historic artefacts has been presented, instead this is built on an ad hoc local 'good enough' concept of symbology design - which is exactly the problem that has led us to Too many icons #3635 in the first place.
  • our symbol and label prioritization is essentially broken (Symbol and label prioritization is not in sync with the starting zoom levels #3880) and adding more baggage to it without actually fixing it will only aggravate the problem.
  • we currently have no consensus on the future of point symbols in this style as shown in Too many icons #3635 and discussion on this confirms this lack of consensus is still ongoing. We need to resolve this and develop common goals and a common vision for the future of POI symbol display and just adding more of them does not help in that regard.

This is all obviously not your fault in developing this. Generally speaking choosing a POI symbol addition for the first PR is rarely a good idea because at the current state of this style this is a very difficult thing to do well.

@imagico
Copy link
Collaborator

imagico commented Oct 15, 2020

@kocio-pl - please document labels with a meaning that is not intuitively clear to everyone.

@kocio-pl
Copy link
Collaborator

What do you mean, is there any documentation for tags?

@imagico
Copy link
Collaborator

imagico commented Oct 15, 2020

All labels can have a description: https://github.com/gravitystorm/openstreetmap-carto/labels

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 21, 2020

While waiting for @alexriabtsev regarding icon rescaling and test rendering (please post a comment if you need some help), just some quick comments:

  1. No icon is perfect, this is symbolic by definition. Let's look at your own recently merged icon design as an example:

https://github.com/gravitystorm/openstreetmap-carto/blob/master/symbols/office/consulate.svg

What does it represent? Is this some office=government? Some secretary of town hall maybe? It's not clear at all. Historic aircraft icon is far more solid in that respect, that it shows an actual physical object, not the abstract institution.

  1. Prioritizing things is about tuning things, not a goal in itself. Moreover Symbol and label prioritization is not in sync with the starting zoom levels #3880 is just a general idea which has no guidelines what is more important and why, let alone having the code, so it might take next months or years before it launches - if ever. That is not a convincing reason for everybody else to hold on, especially when the code for this one is almost ready.

  2. Too many icons #3635 is similar - the fact, that some people have some general ideas written, is still just a draft as 400+ other open tickets, not even a guideline. I have also just found out that you are inconsistent in how do you treat your own PR (moving from amenity=embassy to office=diplomatic and differentiating embassy and consulate #4168) and what others try to do - it introduces a new icon and there's not even a mention about this ticket. Could you please explain that?

@imagico
Copy link
Collaborator

imagico commented Oct 21, 2020

Please keep the discussion here on topic. If you want to discuss consensus based decision making we have #3951, if you want to discuss strategy for POI symbols rendering we have #3635. If you need clarification on what #3880 is about just ask there. And if you want to bring up a topic that is not part of any of those topics and that is not of concern to this PR please open a new issue.

@kocio-pl
Copy link
Collaborator

I just replied to the topics you have introduced and as shortly as you, so I guess it makes sense here. If you think only 1. is relevant, please don't start any other topics to begin with.

@alexriabtsev
Copy link
Author

While waiting for @alexriabtsev regarding icon rescaling and test rendering (please post a comment if you need some help), just some quick comments:
yeah, I would appreciate help! thanks in advance!

@kocio-pl
Copy link
Collaborator

What help do you need exactly? Please let me know.

Resizing image in Inkscape should be easy, probably this could help you:

https://designbundles.net/design-school/how-to-resize-an-image-in-inkscape

I also believe changing numbers from 15 to 14 in SVG in text editor might work, but I just have not tested it.

When it comes to testing environment, we have instructions, and I believe Docker installation should be the easiest:

https://github.com/gravitystorm/openstreetmap-carto/blob/master/DOCKER.md

@alexriabtsev
Copy link
Author

OK, I resized svg

@kocio-pl
Copy link
Collaborator

Great! What about test rendering? Do you need some more support?

@alexriabtsev
Copy link
Author

image

I guess everything is fine?

@kocio-pl
Copy link
Collaborator

This is only about basic code consistency. What we need now is to make visual test how that change really works, so you need to import relevant area with historic aircrafts (you can easily export that data from small area - use https://www.openstreetmap.org/export or some country data where you can find them) and look here:

https://github.com/gravitystorm/openstreetmap-carto/blob/master/DOCKER.md#test-rendering

@alexriabtsev
Copy link
Author

OK, I'll try to. Never done this before

@pnorman pnorman marked this pull request as draft September 11, 2021 18:54
@pnorman
Copy link
Collaborator

pnorman commented Jun 20, 2022

Closing as abandoned - we never got a test rendering.

@pnorman pnorman closed this Jun 20, 2022
@aceman444
Copy link

That is sad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted tag for valid PRs needed for https://hacktoberfest.digitalocean.com new features Requests to render new features POI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Render historic=* POI's
7 participants