-
Notifications
You must be signed in to change notification settings - Fork 199
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
Conversation
Added roof height option
Added roof shape to the presets based on entries found in https://wiki.openstreetmap.org/wiki/Key:roof:shape
🍱 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. |
As a 3D renderer developer, I would prefer if the list of |
We have a couple of options here.
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. |
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. |
Thanks. FYI: Next step would be to comment with "Checklist and Test-Documentation Template" |
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)
There was a problem hiding this 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.
"roof/shape", | ||
"roof/height", |
There was a problem hiding this comment.
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…
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 |
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]>
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. Existing feature example: |
Removed clarification/confusion extra and left just simple label name
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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")
- Update the https://wiki.openstreetmap.org/wiki/Item:Q1839 on this as well
- Remove one value ("saltbox") that was disputed in the Wiki
I created ideditor/schema-builder#15 to talk about icons.
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:
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:
After you submit your PR, the system will create a preview and comment on your PR:
Once the preview is ready, use it to test your changes.
Now copy the snippet below into a new comment and fill out the blanks.
Now your PR is ready to be reviewed.