Modify

Opened 6 months ago

Closed 6 months ago

Last modified 5 months ago

#16189 closed enhancement (fixed)

[PATCH] Introduce almost square check for buildings.

Reported by: marxin Owned by: team
Priority: major Milestone: 18.04
Component: Core validator Version: latest
Keywords: square angle building Cc: bjohas

Description

I consider it very handy to detect almost square buildings for which users are supposed to use Square command.

Attachments (2)

0002-Introduce-almost-square-check-for-buildings.patch (6.1 KB) - added by marxin 6 months ago.
0001-Introduce-almost-square-check-for-buildings.patch (10.2 KB) - added by marxin 6 months ago.
Update version

Download all attachments as: .zip

Change History (30)

comment:1 Changed 6 months ago by Don-vip

Thanks for this second patch :)
Some comments:

  • getNormalizedAngleInDegrees(double angle) could be a new public (static) method of the Geometry class. Also you can return the result directly, no need to change angle value.
  • please always add javadoc for public methods. It's hard to guess what is returned by the getAngles method
  • you can add @since xxx in new classes and new public methods of existing classes. We replace the xxx by the revision number when applying the patch
  • it would be great to also add a unit test for this new check

Changed 6 months ago by marxin

Update version

comment:2 in reply to:  1 Changed 6 months ago by marxin

Replying to Don-vip:

Thanks for this second patch :)
Some comments:

  • getNormalizedAngleInDegrees(double angle) could be a new public (static) method of the Geometry class. Also you can return the result directly, no need to change angle value.
  • please always add javadoc for public methods. It's hard to guess what is returned by the getAngles method
  • you can add @since xxx in new classes and new public methods of existing classes. We replace the xxx by the revision number when applying the patch
  • it would be great to also add a unit test for this new check

All notes should be included in new version of the patch.

comment:3 Changed 6 months ago by Klumbumbus

Component: CoreCore validator

comment:4 Changed 6 months ago by Klumbumbus

Ticket #14952 has been marked as a duplicate of this ticket.

comment:5 Changed 6 months ago by Klumbumbus

Cc: bjohas added

comment:6 Changed 6 months ago by Don-vip

Keywords: square angle building added
Milestone: 18.04

comment:7 Changed 6 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 13670/josm:

fix #16189 - Add "almost square check" for buildings (patch by marxin, modified)

comment:8 Changed 6 months ago by Don-vip

Priority: normalmajor

Thanks a lot! This is a great check! I have modified the patch a little bit (code style + add an autofix).

comment:9 in reply to:  8 Changed 6 months ago by marxin

Replying to Don-vip:

Thanks a lot! This is a great check! I have modified the patch a little bit (code style + add an autofix).

Thanks for review, it was very fast and helpful!

comment:10 Changed 6 months ago by aseerel4c26

Thanks for this feature idea - interesting, in principle!

I think there is an issue here: https://www.openstreetmap.org/way/237778620 this object is flagged with a warning. However, auto-fixing this object moves the nodes by less than the JOSM object history window can display as coordinates (7 decimal places) and it displays "< 0.01 m" difference. Such very very very minor non-square buildings should not be flagged (creates more noise than benefit). I would not like to see edits for such square buildings. Would that non-change be saved in the OSM database as anything but a useless changeset (coordinates are only 7 decimal places in the API)?

Another thought: how do we deal with buildings which are just not exactly square in reality? They will show up as warning forever - until some JOSM validator user comes by and autofixes it.

Version 0, edited 6 months ago by aseerel4c26 (next)

comment:11 Changed 6 months ago by aseerel4c26

Resolution: fixed
Status: closedreopened

comment:12 Changed 6 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 13685/josm:

fix #16189 - increase right angle min threshold to 0.025

comment:13 in reply to:  10 ; Changed 6 months ago by Don-vip

Replying to aseerel4c26:

Another thought: how do we deal with buildings which are just not exactly square in reality?

Let's see with real-world examples until we adjust the test default parameters properly.

comment:14 in reply to:  13 ; Changed 6 months ago by aseerel4c26

Thanks for the first adjustment!

I have experimented a bit with validator.RightAngleBuilding.minimumDelta (after your change it is 0.025) the realworld example https://www.openstreetmap.org/way/122238292 where only https://www.openstreetmap.org/node/1366557884 would be moved by one 7th decimal (which is still < 0.01 m difference). At least a setting value of 0.15 prevents this building from being flagged, if I have tested correctly.

Still for other buildings this produces many warnings where the nodes would only be shifted by one digit in the 7th decimal. Could we suppress all warnings where the autofix would move nodes by less than 0.0000002? I think this value is reasonable to require since 0.0000001 may be just a slightly different rounding.

I guess that a validator.RightAngleBuilding.minimumDelta value of rather close to 1 instead of close to 0 might be fine for productive use, yes, that needs testing.

comment:15 in reply to:  14 Changed 6 months ago by Don-vip

Replying to aseerel4c26:

I have experimented a bit with validator.RightAngleBuilding.minimumDelta (after your change it is 0.025) the realworld example https://www.openstreetmap.org/way/122238292 where only https://www.openstreetmap.org/node/1366557884 would be moved by one 7th decimal (which is still < 0.01 m difference). At least a setting value of 0.15 prevents this building from being flagged, if I have tested correctly.

Thanks for testing! :)

Could we suppress all warnings where the autofix would move nodes by less than 0.0000002?

No so easy. It would require to simulate the squaring operation. It would have a severe performance impact. I prefer tuning the default values a little bit until we're happy with them :)

comment:16 Changed 6 months ago by Don-vip

In 13688/josm:

see #16189 - increase right angle min threshold to 0.1

comment:17 Changed 6 months ago by Klumbumbus

The corner at osmwww:node/2100444620 of osmwww:way/254067656 is reported by the validator as nearly 90° which is fine. However hitting Q or the fix button "destroys" the shape of the building as there is a double 135° corner around osmwww:node/2598976579

So I fear a bit users using the fix button without carefully checking what will or did happen with the whole building (and possible even attached buildings with sharing nodes too).

comment:18 Changed 6 months ago by Klumbumbus

Last edited 6 months ago by Klumbumbus (previous) (diff)

comment:19 in reply to:  17 ; Changed 6 months ago by Don-vip

Replying to Klumbumbus:

hitting Q or the fix button "destroys" the shape of the building as there is a double 135° corner

Ah, good catch. The autofix can only be applied (for now) when all angles are almost right angles.

Last edited 6 months ago by Don-vip (previous) (diff)

comment:20 in reply to:  16 Changed 6 months ago by Klumbumbus

Replying to Don-vip:

In 13688/josm:

see #16189 - increase right angle min threshold to 0.1

I think 0.1 is still a bit low. osmwww:way/574083092 is reported, however Q and ctrl+H gives no change in the coordinates of the four nodes and a distance <0.01m
The change at Q is visible in JOSM only at zoomlevel 24 and higher (osm carto ends at z19!) (standard dpi screen)

comment:21 in reply to:  19 Changed 6 months ago by Klumbumbus

Replying to Don-vip:

Replying to Klumbumbus:

hitting Q or the fix button "destroys" the shape of the building as there is a double 135° corner

Ah, good catch. The autofix can only be applied (for now) when all angles are almost right angles.

Not sure if I understand you right, but I can hit the fix button at this building.

comment:22 Changed 6 months ago by Don-vip

In 13689/josm:

see #16189 - do not alter non-orthogonal building shapes

comment:23 Changed 6 months ago by Don-vip

Sorry I was not clear. I meant that was what JOSM should do, not what it was doing. Now it's better :)

Last edited 6 months ago by Don-vip (previous) (diff)

comment:24 Changed 6 months ago by aseerel4c26

For the cases

https://www.openstreetmap.org/way/238067628 true positive
https://www.openstreetmap.org/way/250085807 false positive
https://www.openstreetmap.org/way/250085814 false positive

at least 0.1375 seems to be a good minimum. The current 0.1 is far too small, in my opinion - compare my #comment:14 . I think too many false positives make this validator rule far less useful - and will create may useless history entries in object histories (I guess the API will still save such a change - even if coordinates did not change).

comment:25 Changed 6 months ago by Klumbumbus

180° angles would be OK too for autofixable buildings, e.g. osmwww:way/200056426

comment:26 Changed 6 months ago by Don-vip

In 13690/josm:

fix #16189 - increase right angle min threshold to 0.25 + allow 180° angles

comment:27 Changed 5 months ago by Don-vip

In 13721/josm:

fix #16264, see #16188, see #16189 - reduce validator false positives:

  • increase tolerance of buildings with almost right angle to 1 degree
  • detect crossing residential areas only with themselves and buildings
  • fix crossing railways messages

comment:28 Changed 5 months ago by Klumbumbus

maxAngleDelta = Config.getPref().getDouble("validator.RightAngleBuilding.maximumDelta", 10.0);
minAngleDelta = Config.getPref().getDouble("validator.RightAngleBuilding.minimumDelta", 1.0);

That means all angles within these two ranges 80°-89° and 91°-100° are reported, right?

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.