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

add amenity=research_institute #4278

Closed
wants to merge 3 commits into from
Closed

add amenity=research_institute #4278

wants to merge 3 commits into from

Conversation

LaoshuBaby
Copy link

Fixes:

Fixes [#2077 ]

Fixes [#4277 ](Duplicate of #2077 )

Changes proposed in this pull request:

Before

Before

Reason

amenity=research_institute be rendered like amenity=university has been calling for a long time, and many contributor in that issue think about it.

Global usage is already growing faster. This year the number of uses has doubled (now ~ 1.5k).

https://github.com/gravitystorm/openstreetmap-carto/issues/2077#issuecomment-744006486


It is the first time I PR to a so large repo so if there are something mistake, please comment to me! Please! Thanks!

@imagico
Copy link
Collaborator

imagico commented Jan 6, 2021

Technically the change seems to be fine (though i have not tested it) - but note the remarks made in #2077 (comment).

@LaoshuBaby
Copy link
Author

Technically the change seems to be fine (though i have not tested it) - but note the remarks made in #2077 (comment).

Yes, all comments about amenity=research_institute can't avoid talking with #2077 , and my problem is also inspired by #2077 because it had last for years!

But I don't know how to deploy a render server on my own computer so I can't give a AFTER version. So I want this PR can be judge by more active-contributors. Thank you for your review!

@polarbearing
Copy link
Contributor

I'm in favour of that rendering which is overdue now.
@LaoshuBaby - did you set up your test environment so you can show the 'After' image next to your 'Before'? Testing is needed.

@@ -2286,6 +2287,9 @@
[feature = 'amenity_college'],
[feature = 'amenity_university'] {
text-fill: darken(@societal_amenities, 80%);
}
[feature = 'amenity_research_institute'] {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can merge this entry with the block of kindergarten, school, college and university.

Copy link
Author

Choose a reason for hiding this comment

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

Emmm……but kindergarten or school are separated, so I think institute should be like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are they?
image

Copy link
Author

Choose a reason for hiding this comment

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

Yes
I‘m new in this repo.
I guess your expected code is like that?

      [feature = 'amenity_kindergarten'],
      [feature = 'amenity_school'],
      [feature = 'amenity_college'],
      [feature = 'amenity_university' || feature = 'amenity_research_institute'] {
        text-fill: darken(@societal_amenities, 80%);
      }

(I don't know whether those is Grammatical (T_T))

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess your expected code is like that?

No:

[feature = 'amenity_school'],
      [feature = 'amenity_college'],
      [feature = 'amenity_university'],
      [feature = 'amenity_research_institute'] {

Cartocss is based on CSS syntax.

Copy link
Author

Choose a reason for hiding this comment

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

Edited.Thanks @HolgerJeromin

@jeisenbe
Copy link
Collaborator

@LaoshuBaby thank you for this PR.

As @HolgerJeromin has mentioned, there is a small technical change that needs to be made in the code to make it a bit shorter.

It is also important to review the changes yourself by rendering the map. While this can be done by the maintainers, we would like contributors to check how the change will look on the map of their own area and some other example locations. This can bring out problems with the current use of the tag, as mentioned in this commont: #2077 (comment)

We need to make sure this tag is being used in a consistent way.

If you don’t know how to set up the rendering server, there are detailed step-by-step instructions for doing this with Docker at this comment: #2291 (comment)

Copy link
Collaborator

@jeisenbe jeisenbe left a comment

Choose a reason for hiding this comment

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

Move line 2291 up 3 lines, between amenity_school and amenity_college

@jeisenbe jeisenbe added the new features Requests to render new features label Jan 10, 2021
@LaoshuBaby
Copy link
Author

@LaoshuBaby thank you for this PR.

As @HolgerJeromin has mentioned, there is a small technical change that needs to be made in the code to make it a bit shorter.

It is also important to review the changes yourself by rendering the map. While this can be done by the maintainers, we would like contributors to check how the change will look on the map of their own area and some other example locations. This can bring out problems with the current use of the tag, as mentioned in this commont: #2077 (comment)

We need to make sure this tag is being used in a consistent way.

If you don’t know how to set up the rendering server, there are detailed step-by-step instructions for doing this with Docker at this comment: #2291 (comment)

Move line 2291 up 3 lines, between amenity_school and amenity_college

For those 2 comments:

Yes, as @jeisenbe said. Maybe someone will render according to those commit out of sympathy. But before asking others for help, I think I should try it myself first. I had tried to configure the rendering server myself before but failed. This time I will do as #2291 like.
Besides, the previous technical problem has been re-committed, please correct me if there are other irregularities.

[feature = 'amenity_kindergarten'],
[feature = 'amenity_college'],
[feature = 'amenity_research_institute'],
[feature = 'amenity_university'] {
Copy link
Contributor

Choose a reason for hiding this comment

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

You changed the order of the lines (kindergarten, school are swapped and the new entry is between collage and university). They are now different from the list above. Best to stay in one order.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@LaoshuBaby could you address the comment above?

@jeisenbe
Copy link
Collaborator

@LaoshuBaby have you been able to test the rendering of this PR?

@jeisenbe
Copy link
Collaborator

Unfortunately, based on my research of how this tag is currently used - in #2077 (comment) - I am not convinced that this tag is being used in a consistent way, or that it would make sense to render the area the same as schools and universities.

@LaoshuBaby
Copy link
Author

Good luck

Next time will work on https://wiki.openstreetmap.org/wiki/Tag:tourism%3Dhotel

@LaoshuBaby LaoshuBaby closed this Feb 28, 2021
@polarbearing
Copy link
Contributor

Strange procedure @jeisenbe.
First you encourage a contributor to provide test renderings, just an hour later you discourage the PR in general.

@geozeisig
Copy link

The number of applications is now over 3500. Please open the proposal so that it can be implemented soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new features Requests to render new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants