Modify

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#5933 closed enhancement (fixed)

[Patch] Building presets

Reported by: bilbo Owned by: team
Priority: normal Milestone:
Component: Core Version: latest
Keywords: Cc:

Description (last modified by !i!)

Patch that will enhance presets for building parameters (height, levels, etc ....)

Note that optimally, these presets should be available for any building (building=house, building=train_station, etc ...) - is this possible to somehow change the '<key key="building" value="yes" />' so that "yes" will be default, but user will be able to choose other building types, and this preset would be available for any way tagged with building key?

Attachments (5)

building-presets.patch (3.2 KB) - added by bilbo 8 years ago.
Patch to add building presets to JOSM
building-presets.2.patch (3.6 KB) - added by bilbo 8 years ago.
Fixed and simplified building presets
5933.patch (28.3 KB) - added by simon04 7 years ago.
5933_v2.patch (28.3 KB) - added by simon04 7 years ago.
5933_v3.patch (28.9 KB) - added by simon04 7 years ago.

Download all attachments as: .zip

Change History (28)

Changed 8 years ago by bilbo

Attachment: building-presets.patch added

Patch to add building presets to JOSM

comment:1 Changed 8 years ago by stoecker

The "select building type" should be possible using a combobox with a default value "yes" and a delete_if_empty set to false.

comment:2 Changed 8 years ago by tordanik

This preset introduces several tags that are controversial, haven't been discussed sufficiently and/or don't work for most cases. For example:

  • building:roof:orientation isn't properly defined for anything but the most simple outlines
  • objections against culture-dependent understanding of building:min_level have been raised on the proposal's discussion page and not been addressed so far
  • building:architecture provides an arbitrary set of values and isn't actually popular in tagging practice
  • building:type is mostly a duplicate of building

I wouldn't be happy if these ideas were prematurely established via JOSM preset. These issues need to be discussed more thoroughly.

comment:3 Changed 8 years ago by !i!

You are right, but we all would be very happy if you would add the well known/established ones already.

comment:4 Changed 8 years ago by tordanik

The only keys in this template which I would consider truly established are building, height and building:levels. If you want to add these, you have my support.

comment:5 Changed 8 years ago by bilbo

Well, this patch add all the tags mentioned anywhere. Aside from height and building:levels, min_height and/or building:min_level is used in about 1000 buildings in OSM (as buildings with large parts between ground and first floor are not so common), so while not very common, it is quite used. But at least the height and building:levels should be added now and the list can then be extended later once some further tags are considered stable ....

stoecker> "select building type" should be possible using a combobox

But problem is that the buuilding type is a key:

<key key="building" text="Building type" value="yes" />

Is it possible to specify something like:

<key-as-a-combo key="building" text="Building type" values="yes,house,commercial,retail, ......." default="yes" delete_if_empty="true" />

instead?

Documentation does not mention such possibility.

comment:6 Changed 8 years ago by stoecker

You can simply change the key to a combo. You must only ensure, that the value is set always (thus delete_if_empty=false). Key means "set without user interaction". All the other forms mean "set with user-interaction".

comment:7 Changed 8 years ago by stoecker

Summary: [patch] Building presets[patch needs rework] Building presets

comment:8 Changed 8 years ago by bilbo

I tried this:

<key key="building" value="yes" />
<combo key="building" text="Type of building" values="train_station,hangar,yes" default="yes" use_last_as_default="true"/>

And when selecting from menu, it works fine. However, if you select existing building (anything tagged with building=yes), then in the panel with tag editor you click "Man made/man made/Building" and change the "Type of building" to "train_station", the tag will change to building=tran_station (so far all is ok), but then "Man made/man made/Building" will disappear from the panel with tags (as building=train_station does not match the key), so you are unable to edit the building using this preset anymore.

I loooked in the corresponding Java code in tag editor and it seems there is currently no way of showing this presets in the panel with tag editor for building=<anything> (as ).
So I guess some code change would be required for this preset to fully work (enhance the key to and then support this in the preset matching logic org/openstreetmap/josm/gui/dialogs/properties/PresetListPanel.java).

Changed 8 years ago by bilbo

Attachment: building-presets.2.patch added

Fixed and simplified building presets

comment:9 Changed 8 years ago by bilbo

Summary: [patch needs rework] Building presets[patch] Building presets

Attached reworked patch - only height and building:levels tags from the advanced attributes are present (the rest can be added later) and you can now specify building type. Only three are present (hanger, train_station and the default "yes")

More types can be added, but I am not sure of what values should end up there as being considered well defined, etc ...

I suggest adding most of : hut, house, apartments, residential, industrial (these have at least 10000 occurences on entire planet)
and perhaps think about: garage, garages, barn, shed, university, bunker, cabin, chapel, church, commercial, factory, farm, greenhouse, hall, hospital, industrial, office, retail, school, tank, warehouse (some of the others I've seen with at least few hundred occurences)

Minor code change was needed, so the preset will show up in the tag editor as clickable for any building tag, not only for default "building=yes"

comment:10 Changed 8 years ago by stoecker

Summary: [patch] Building presets[patch needs rework] Building presets

I don't like that both key and combo describe the same key. This is error prone. Also you match_only_key is more a workaround than a clean solution.

Your idea to change code seems sensible, but rather than your solution I would a suggest a "match" property, which can be used for all types (especially combo boxes), which has following values:

  • yes (the default for keys, matches key+value)
  • no (does not match - disable key matching, default for all other types)
  • key (what you implemented)

For buildings presets, the key line can then be dropped and match="key" can be added to the combo. match="yes" for combos would mean one of the values matches.

comment:11 Changed 8 years ago by simon04

Type: defectenhancement

comment:12 Changed 8 years ago by !i!

Description: modified (diff)

We are currently bringing the 3D tags together, there are just to much: http://wiki.openstreetmap.org/wiki/3D_Development/Tagging#Usage_tools
Hopefully we can do it on the 3D meeting next year: http://wiki.openstreetmap.org/wiki/2nd_3D_Workshop_Garching

comment:13 Changed 8 years ago by stoecker

Summary: [patch needs rework] Building presets(patch needs rework) Building presets

comment:14 Changed 7 years ago by simon04

Summary: (patch needs rework) Building presets[Patch] Building presets

Patch attached. It comes with a refactoring of the matching process (removes some duplicate code and some magical arithmetic ("we subtract 100 if not found and add 1 if found").

I propose to have 4 match types (included in the patch):

private enum MatchType {
     /**
     * Neutral, i.e., o not consider this item for matching.
     */
    NONE("none"), /**
     * Positive if key matches, neutral otherwise.
     */
    KEY("key"),
    /**
     * Positive if key matches, negative otherwise.
     */
    KEY_REQUIRED("key!"),
    /**
     * Positive if key and value matches, negative otherwise.
     */
    KEY_VALUE("keyvalue");
/* ... */
}

In the same run, I suggest to deprecate and eventually remove delete_if_empty as this is set to true in all cases except for motorroad (for an unknown reason).

For Text items, I suggest to let default default to "" (i.e., the empty string). The former two adoptions give rise to a more tidy presets file.

The attribute required for all but the Role item is now subsumed by match (see #5903), and therefore has been marked deprecated.

The XML file+schema needs according adaption.

I'm looking forward to hearing some comments. :-)

Version 0, edited 7 years ago by simon04 (next)

Changed 7 years ago by simon04

Attachment: 5933.patch added

Changed 7 years ago by simon04

Attachment: 5933_v2.patch added

comment:15 Changed 7 years ago by simon04

A small update to fix a NullPointerException. Any comments on these (suggested) changes?

Changed 7 years ago by simon04

Attachment: 5933_v3.patch added

comment:16 Changed 7 years ago by simon04

Another bug fix (closed ways haven't been handled correctly).

Removing delete_if_empty="true" and default="" reduces defaultresets.xml by 50kb to ≈350kb.

@team: What do you think of an inclusion of this patch prior the upcoming tested release?

comment:17 Changed 7 years ago by simon04

Resolution: fixed
Status: newclosed

In 5155/josm:

fix #5933 - tagging presets: allow to change the matching process (match=none|key|key!|keyvalue), remove delete_if_empty, default defaults to "", adapted comments in defaultpresets.xml, refactoring of the matching process (removes some duplicate code and some magical arithmetic)

comment:18 Changed 7 years ago by simon04

In 5156/josm:

see #5933 - shrink defaultpresets.xml according to changes in r5155

comment:19 Changed 7 years ago by simon04

In 5157/josm:

see #5933 - update preset schema according to changes in r5155

comment:20 Changed 7 years ago by simon04

In 5167/josm:

see #5933 - update preset schema according to changes in r5155

comment:21 Changed 7 years ago by rickmastfan67

Just spotted a bug that's related to the changes done here in this ticket.

Whenever I activate the "bridge" preset, the following "warnings" show up in the CL:

Warning in tagging preset "bridge-Bridge": Ignoring 'values' attribute as 'list_entry' elements are given.
Warning in tagging preset "layer-Layer": Ignoring 'values' attribute as 'list_entry' elements are given.
Warning in tagging preset "incline-Incline": Ignoring 'values' attribute as 'list_entry' elements are given.

This is happening in r5171. Just thought I would mention this as it's the only preset that has been throwing up any "warning" in the CL when I use it so far since r5155.

comment:22 Changed 7 years ago by simon04

In 5172/josm:

see #5933 - avoid warnings "Ignoring * attribute as * elements are given"


In fact related to #7552 and not this ticket.

Last edited 7 years ago by simon04 (previous) (diff)

comment:23 Changed 7 years ago by simon04

In 5180/josm:

see #5933 - update preset schema, comments in defaultpresets.xml according to changes in r5155

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.