Modify

Opened 3 months ago

Closed 5 weeks ago

Last modified 5 weeks ago

#20038 closed enhancement (fixed)

Autofix in RightAngleBuildingTest should be removed

Reported by: GerdP Owned by: team
Priority: normal Milestone: 20.12
Component: Core validator Version:
Keywords: right angle building Cc:

Description

I don't see how the implemented autofix improves the data. The Fix action moves one node and often enough it just creates another "almost square angle" doing this. So, if you click on Fix and execute validator again you get another bunch of "Building with an almost square angle", typically nearly the same number, sometimes even higher.

Attachments (2)

rectify.osm (9.5 KB) - added by mdk 2 months ago.
rectify2.osm (3.1 KB) - added by mdk 2 months ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 3 months ago by skyper

+1

I actually, do not know what I should think of this test. I like it and have my problems with it.

Regarding problems I have:

  • It needs an option to specify the offset when to warn or make it tighter. I get warnings about 85° which in my eyes should not be warned.
  • It only warns/highlights one node even if there are more within the same object.
  • Ignore is dangerous as:
    • does not check the whole building anymore not only the corner node
    • if the building is changed, again, the whole way will not be check again.

comment:2 Changed 3 months ago by GerdP

Check expert options validator.RightAngleBuilding.maximumDelta and validator.RightAngleBuilding.minimumDelta

I've completely disabled the test. I see no way to detect if the real angle is closer to 90° or to 85°. In my hometown buildings have non-square angles. I grew up in one, but it is raized now.

comment:3 Changed 3 months ago by Klumbumbus

+1 for removing the autofix, we have the Q-tool in case you want to fix it.
Also the autofix also moves nodes which are part of e.g. other buildings, which might be wanted or unwanted.

comment:4 Changed 3 months ago by GerdP

It only warns/highlights one node even if there are more within the same object.

It can be changed that so that all relevant nodes in a way are hilighted. Or just the way?

comment:5 Changed 2 months ago by mdk

There is a method to fix a single angle - which often result in a new warning on another angle.
There is a method to rectify all angels of a way.

But is there a possibility to fix only some angels? I would expect a method where I could select some nodes and optional one way (or even more) and rectify them.

Use case: way 287389098 and way 196756607

Last edited 2 months ago by skyper (previous) (diff)

comment:6 Changed 2 months ago by GerdP

You can select a series of nodes in one way and press Q.

Changed 2 months ago by mdk

Attachment: rectify.osm added

comment:7 Changed 2 months ago by mdk

Hmmmm. With the both example ways the result is far away from expected.
In the attached example rectify.osm you see on top the two ways. I add a fixme to all nodes I want to rectify. Select these nodes and pressing Q produce the both results below.
Both results are completely different! I think this has to do with order I select the nodes. But both results are absolut crap!

comment:8 in reply to:  4 Changed 2 months ago by skyper

Replying to GerdP:

Check expert options validator.RightAngleBuilding.maximumDelta and validator.RightAngleBuilding.minimumDelta

Thanks. There are quite some undocumented advanced preferences.

I've completely disabled the test. I see no way to detect if the real angle is closer to 90° or to 85°. In my hometown buildings have non-square angles. I grew up in one, but it is razed now.

It depends on the area, the age of the buildings and additionally the available sources. In my city, I have a mix of all.

Replying to GerdP:

It only warns/highlights one node even if there are more within the same object.

It can be changed that so that all relevant nodes in a way are highlighted. Or just the way?

All relevant nodes, though, I would like to ignore individual nodes and not the whole building, but I did not figure out, how to only have one warning per building with all relevant nodes but be able to ignore only some of the nodes.

comment:9 in reply to:  7 Changed 2 months ago by GerdP

Replying to mdk:

Hmmmm. With the both example ways the result is far away from expected.
In the attached example rectify.osm you see on top the two ways. I add a fixme to all nodes I want to rectify. Select these nodes and pressing Q produce the both results below.
Both results are completely different! I think this has to do with order I select the nodes. But both results are absolut crap!

Yes, order seems to matter. I keep Ctrl pressed and select the nodes in the order in which they appear on the way. Reverse order also works.

comment:10 Changed 2 months ago by GerdP

For way 196756607 I start with node 2456342151 and select all nodes in clockwise order until I reach node 2070494715.
Result looks OK to me.

comment:11 Changed 2 months ago by mdk

The problem in the example is, that there are 3 "clusters" of nodes with 90 degrees which are separated by 45 degree angels.

  1. Never select nodes of different "clusters"
  2. you can't rectify nodes depending to more than one way (adjusting buildings). So you need 2 attempts, where the second destroy the angels of the first again.

comment:12 Changed 2 months ago by GerdP

Yes, that's why I don't care that much about right angles ;)

Changed 2 months ago by mdk

Attachment: rectify2.osm added

comment:13 Changed 2 months ago by mdk

see: rectify2.osm
At least I want the B1-B6 and C1-C4 rectified together. But this doesn't work as expected.

But it would be possible with a new action:

  1. Select a way for this action
  2. Take all nodes from the way which have a 90 or 180 degree angel (including "almost")
  3. Separate these nodes into *clusters" which are separated by clearly not 90 or 180 degree angels.
  4. Sort the nodes in each cluster
  5. Execute "rectify" on each cluster

This would also solve the original issue: This new action could easily be used as an Autofix for this situation.

This could also be generalised for selecting multiple ways.

comment:14 Changed 2 months ago by GerdP

The question is how this improves the data. In the end you may have correct angles but lengths are wrong.

comment:15 Changed 2 months ago by skyper

Component: CoreCore validator
Keywords: right angle building added

comment:16 Changed 2 months ago by mdk

The question is how this improves the data. In the end you may have correct angles but lengths are wrong.

The length of segments will be change slightly by this operation - The same as rectifying the whole way.

There are infinite solution for the rectifying operation. So we need some additional goals for choosing one:

  • minimize movement of nodes
  • minimize length changes of segments
  • minimize size change of the area.

A "natural" behaviour would be something like "keep the size of the area and try to move nodes as little as possible". I don't know how rectifying a whole way actually works. But this action works as I expect. But rectifying nodes looks like to have a complete different implementation.

What is the semantic of different selection orders of the nodes? Can you explain a user what does it mean to select nodes in a different way and what result he could expect? If not, the action should not produce different results depending of the selection order. So the action should sort the nodes at the beginning.

The rectify action should produce the same results if applied on a way or if applied on all nodes selected of a way. Especially when both have the same shortcut. But this is absolutely not the case! Draw a nearly rectangle closed way with 4 nodes. make some copies of that way.
Select a way and press Q. The result is a "natural" rectified way with all angels having 90 degree.
Select all nodes of a way and press Q. I would expect the same result as above, but this doesn't happens. Depending on the selection order I get:

  • a way where not all angels are rectified.
  • an area with size 0 (only a line is left)
Last edited 2 months ago by mdk (previous) (diff)

comment:17 Changed 2 months ago by GerdP

I agree regarding the possible goals.
I've never looked at the details of the implementation of the OrthogonalizeAction. The code is quite complex.
We have several actions which do different things depending on the actual selection, that's intented.
The current code seems to do different things when

  • selection contains only ways
  • selection contains one node and one or more ways (the selected node is moved)
  • selection contains two nodes and one or more ways (I don't yet understand what the selected nodes mean)
  • selection contains no ways and three or more nodes (I think it first creates a virtual unclosed way containing the nodes in the given order and does the same as if the selection contains a way with those nodes)
Last edited 2 months ago by GerdP (previous) (diff)

comment:18 in reply to:  17 Changed 2 months ago by Klumbumbus

Replying to GerdP:

  • selection contains two nodes and one or more ways (I don't yet understand what the selected nodes mean)

You can specify a direction, see wiki:/Help/Action/OrthogonalizeShape#Usingtwonodesforalignment (and wiki:/Help/Action/OrthogonalizeShape#Complexbuildings and wiki:/Help/Action/OrthogonalizeShape#OrthogonalizationisnotRotation)

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

comment:19 Changed 2 months ago by GerdP

Ah, thanks. I think the case with "one node and one or more ways selected" was added for Autofix in RightAngleBuildingTest.

comment:20 Changed 2 months ago by GerdP

I think "direction" mislead me. I thought it refers to the order of the nodes in the way ;)

comment:21 Changed 5 weeks ago by GerdP

Resolution: fixed
Status: newclosed

In 17405/josm:

fix #20038: Autofix in RightAngleBuildingTest should be removed

  • remove code which creates a rather useless move command

comment:22 Changed 5 weeks ago by GerdP

Milestone: 20.12

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.