Skip to content

Add roof shape and roof height fields to building presets #1552

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

Merged
merged 13 commits into from
Jun 6, 2025

Conversation

trs998
Copy link
Contributor

@trs998 trs998 commented May 20, 2025

Description, Motivation & Context

openstreetmap/iD#1829 - roof type. This adds roof shape and roof height, as this currently required manual entry of the tags in iD. Data would be more consistent with presets.

Related issues

openstreetmap/iD#1829 - opened back when tags were internal to iD

Links and data

(https://wiki.openstreetmap.org/wiki/Key:roof:shape)

Relevant tag usage stats:


Roof shape is tagged 8.3 million times - https://taginfo.openstreetmap.org/keys/roof%3Ashape

Checklist and Test-Documentation Template

Read on to get your PR merged faster…

Follow these steps to test your PR yourself and make it a lot easier and faster for maintainers to check and approve it.

This is how it works:

  1. After you submit your PR, the system will create a preview and comment on your PR:

    🍱 You can preview the tagging presets of this pull request here.
    If this is your first contribution to this project, the preview will not happen right away but requires a click from one of the project members. We will do this ASAP.

  2. Once the preview is ready, use it to test your changes.

  3. Now copy the snippet below into a new comment and fill out the blanks.

  4. Now your PR is ready to be reviewed.

## Test-Documentation

### Preview links & Sidebar Screenshots

<!-- Use the preview to find examples, select the feature in question and **copy this link here**.
     Find examples of nodes/areas. Find examples with a lot of tags or very few tags. – Whatever helps to test this thoroughly.
     Add relevant **screenshots** of the sidebar of those examples. -->

<!-- FYI: What we will check:
     - Is the [icon](https://github.com/ideditor/schema-builder/blob/main/ICONS.md) well chosen.
     - Are the fields well-structured and have good labels.
     - Do the dropdowns (etc.) work well and show helpful data. -->

### Search

<!-- **Test the search** of your preset and share relevant **screenshots** here.
     - Test the preset name as search terms.
     - Also test the preset terms and aliases as search terms (if present). -->

### Info-`i`

<!-- **Test the info-i** for your fields and preset and share relevant **screenshots** here.
     The info needs to help mappers understand the preset and when to use it.
     [Learn more…](https://github.com/openstreetmap/id-tagging-schema/blob/main/CONTRIBUTING.md#info-i)
 -->

### Wording

- [ ] American English
- [ ] `name`, `aliases` (if present) use Title Case
- [ ] `terms` (if present) use lower case, sorted A-Z
<!-- Learn more in https://github.com/openstreetmap/id-tagging-schema/blob/main/GUIDELINES.md#2-design-the-preset -->

trs998 added 2 commits May 20, 2025 21:13
Added roof height option
Added roof shape to the presets based on entries found in https://wiki.openstreetmap.org/wiki/Key:roof:shape
Copy link

🍱 Your pull request preview is ready

Please use this preview to check your changes. Ideally use the test documentation template and document your test results by commenting on the PR. This will speed up the review process for everyone.

FYI, once this PR is merged, you can use the iD Editor Preview to test your changes in interaction with all other changes.

@tordanik
Copy link

As a 3D renderer developer, I would prefer if the list of roof:shape values was restricted to those which are also part of Simple 3D Buildings . Some of the extra values (e.g. gabled_height_moved) have unclear semantics when combined with other tags such as roof:direction, and pretty much all of them they were added to the wiki without first forming a consensus with the wider community or with data consumers.

@tordans
Copy link
Collaborator

tordans commented May 21, 2025

I would prefer if the list of roof:shape values was restricted to those which are also part of Simple 3D Buildings

We have a couple of options here.
My suggestion would be to use…

  • only translate some values via the strings property
  • set "autoSuggestions": true, to expose other values

iD will then mix both the translated and raw values which gives users a way to see which are recommended and better documented.

I would also suggest to improve the Wikidata items (see "Info-i" section in the Testing recommendations) for the values to either signal that they are well or baldy defined.

In general, we don't want to take a strong stance on tagging inside the Repo but rather ask to resolve those discussions in the Forum and via Proposals. It sounds like the tags need to be documented better and maybe even deprecated.

@trs998
Copy link
Contributor Author

trs998 commented May 21, 2025

I have revised the commit - I agree the odder roof shapes don't have clear 3d rendering and don't seem used much - so I have shortened the list into the one on simple 3d buildings - as per todans' suggestion.

@tordans
Copy link
Collaborator

tordans commented May 21, 2025

Thanks. FYI: Next step would be to comment with "Checklist and Test-Documentation Template"

trs998 added 3 commits May 21, 2025 09:29
Added hint to use the roof shape and roof height tags as more tags on a building preset
Added hint to use roof shape and height tags
Added hint to use roof shape tags when building is simply a roof (eg fuel station cover on pillars)
Copy link
Collaborator

@tordans tordans 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 the PR. I left a few comments. Please share the testing documentation once you feel this is ready for the next review.

Comment on lines +22 to +23
"roof/shape",
"roof/height",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to be fancy about it, you could extract those three fields into a template to share them, see https://github.com/openstreetmap/id-tagging-schema/tree/main/data/presets/%40templates

That will make it easier to not forget one later…

@tordans
Copy link
Collaborator

tordans commented May 22, 2025

One more thing, those roof shape options would be a great case for field option icons. Some docs on that at https://github.com/ideditor/schema-builder?tab=readme-ov-file#icons
It is used on the crossing markings ATM for example. We can do this in a follow up, though, because I image getting the right Icons going will take a bit.

trs998 and others added 2 commits May 23, 2025 08:13
Change capitalisation from "Roof Height" being the field title

Co-authored-by: Tobias <[email protected]>
Label more direct and less specific

Co-authored-by: Tobias <[email protected]>
@trs998
Copy link
Contributor Author

trs998 commented May 23, 2025

I agree icons would be good. I am concerned people may put the height of the total building in Roof Height rather than specifically the height which is just the roof part, but I think the label field is used for the english version of the field name and doesn't have hints in it. And the documentation seems good, with illustrative pictures.

When I tried the preview, it did not show the new field definitions on a building with roof:shape=something - but it now does - see below screenshots.

image
image

Existing feature example:
https://pr-1552--ideditor-presets-preview.netlify.app/id/dist/#locale=en&map=20.25/52.62865/1.29159&disable_features=boundaries&background=Bing&id=w1332031670
image

Removed clarification/confusion extra and left just simple label name
Copy link
Collaborator

@tordans tordans left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks. I will keep it open for a bit to wait for feedback and merge in a few days.

@tordans tordans added the waiting-ready-to-merge Ready to merge, but let's wait a few days for possible feedback. label May 26, 2025
Copy link
Collaborator

@tordans tordans left a comment

Choose a reason for hiding this comment

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

Labels always start with capital letter; will apply change right away https://github.com/search?q=repo%3Aopenstreetmap%2Fid-tagging-schema%20%22label%22%3A&type=code

@tordans tordans changed the title Roof Shape and Roof Height tags Add roof shape and roof height fields to building presets Jun 6, 2025
Copy link
Collaborator

@tordans tordans left a comment

Choose a reason for hiding this comment

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

I will merge this now.

Ich change a view things

  • Use "Label Name" (not "Label name")
  • Add "Roof Height (Meters)" to indicate the default unit (same as "height")
  • Remove one value ("saltbox") that was disputed in the Wiki

I created ideditor/schema-builder#15 to talk about icons.

@tordans tordans merged commit e9f520a into openstreetmap:main Jun 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-field waiting-ready-to-merge Ready to merge, but let's wait a few days for possible feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants