Modify

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#8431 closed defect (fixed)

"Create circle" changes way direction

Reported by: naoliv Owned by: team
Priority: normal Milestone: 14.03
Component: Core Version:
Keywords: create circle Cc: richlv

Description (last modified by Don-vip)

While selecting one node + it's way to create a circle (via the O shortcut) with the nodes properly aligned I saw that Josm is reverting the way direction.

See the attached video and the attached .osm file.

The example roundabout (from the video) is tagged with a FIXME=josm in the osm file.
Note how the way has it's direction reversed.

JOSM is:

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2013-02-13 02:31:45
Last Changed Author: Don-vip
Revision: 5713
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2013-02-12 23:31:18 +0100 (Tue, 12 Feb 2013)
Last Changed Rev: 5713

Attachments (7)

josm-circle.ogv (438.8 KB) - added by naoliv 7 years ago.
Video of the problem
circle-error.osm.bz2 (188.9 KB) - added by naoliv 7 years ago.
Example OSM file. Search for the roudabout tagged with FIXME=josm
bug8431.patch (11.9 KB) - added by Balaitous 6 years ago.
patch from r6892 Order nodes anticlockwise before align in circle, cleanup old code for regular polygon (replaced by regular arc since r6892)
circle_testcase.osm (2.5 KB) - added by richlv 6 years ago.
bug8431_2.patch (11.1 KB) - added by Balaitous 6 years ago.
since r6920
use_case_of_half_circle.jpg (80.1 KB) - added by Balaitous 6 years ago.
circle-uneven.png (3.3 KB) - added by naoliv 6 years ago.

Download all attachments as: .zip

Change History (44)

Changed 7 years ago by naoliv

Attachment: josm-circle.ogv added

Video of the problem

Changed 7 years ago by naoliv

Attachment: circle-error.osm.bz2 added

Example OSM file. Search for the roudabout tagged with FIXME=josm

comment:1 Changed 7 years ago by skyper

Component: PluginCore

comment:2 Changed 7 years ago by Don-vip

In 5719/josm:

see #8431 - Align in circle: Do not allow "regular polygon" mode for way nodes being referred by another primitives. The initial problem (way being reverted) remains.

comment:3 Changed 7 years ago by Don-vip

Priority: minornormal

comment:4 Changed 7 years ago by Don-vip

see also #8425

comment:5 Changed 6 years ago by Don-vip

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

comment:6 Changed 6 years ago by Don-vip

Cc: richlv added

r5719 has introduced a problem: it should be possible to allow regular polygon in that case, see dataset attached to #9290.

comment:7 Changed 6 years ago by Don-vip

Description: modified (diff)

comment:8 Changed 6 years ago by Don-vip

Milestone: 14.02

comment:9 Changed 6 years ago by richlv

for the record, regression reported in #9290 still there in 6767

comment:10 Changed 6 years ago by Don-vip

Milestone: 14.0214.03

Changed 6 years ago by Balaitous

Attachment: bug8431.patch added

patch from r6892 Order nodes anticlockwise before align in circle, cleanup old code for regular polygon (replaced by regular arc since r6892)

comment:11 Changed 6 years ago by Balaitous

Summary: "Create circle" changes way direction[patch] "Create circle" changes way direction

comment:12 Changed 6 years ago by skyper

Keywords: create circle added

comment:13 Changed 6 years ago by skyper

see also #7421

comment:14 Changed 6 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 6919/josm:

fix #8431, fix #9839 - fix issues with "align nodes in circle" action (patch by Balaitous)

comment:15 Changed 6 years ago by richlv

Resolution: fixed
Status: closedreopened

hmm. thanks, but now it does not seem to be consistent - and it's different from the original behaviour :)

a) single closed way
previously :
select way only - circular, but node distribution is not changed
select way + 1 node - circular, nodes evenly distributed
now :
select way only - circular, nodes evenly distributed
select way + 1 node - circular, nodes evenly distributed

b) closed way + one attached way
previously :
select way only - circular, but node distribution is not changed
select way + 1 node - circular, nodes evenly distributed
now :
select way only - circular, nodes evenly distributed
select way + 1 node - circular, nodes... not sure quite what happens here, but it seems to do this :

from the selected node on each side nodes are distributed evenly up to the connected way - so the circle gets uneven distribution. this only seems to happen if some other way attaches at the beginning of segment 1.

see attached testcase. select the rightmost node, press 'o'.

note that selecting two nodes keeps them in place and makes an arc of the remaining, that seems to work properly.


as for the change in behaviour where selecting just the way did not change node distribution - if that was intended, probably documentation should be updated ?

Last edited 6 years ago by richlv (previous) (diff)

Changed 6 years ago by richlv

Attachment: circle_testcase.osm added

comment:16 Changed 6 years ago by naoliv

I like to have the nodes evenly distributed.
Just in case this feature gets reverted/removed, it could at least be made as an option (to have or not an even distribution)

Last edited 6 years ago by naoliv (previous) (diff)

comment:17 Changed 6 years ago by richlv

note that previously nodes were not evenly distributed if you selected only the way and evenly distributed if you selected way and one node on it. i do agree that there must be a way to distribute the nodes evenly :)

comment:18 Changed 6 years ago by Balaitous

As JOSM user, I think it is better to have evenly distributed nodes as often as possible.
So I propose to define the cases where angle of a node (from center) doesn't have to change, and allow change for others.

Nodes that should have fix angle are:

  • Nodes connected to other ways (that's the case in this version)
  • Nodes used by a relation (that's the case in this version)
  • Nodes with tags (what tag ? all including source tag ?) (that's NOT the case in this version)

And nodes between fix angle nodes are evenly distributed.
And after to update documentation.

But there is a more important bug: If you select 2 disjoint ways and >= 4 nodes, it does anything ! I will send a patch tomorrow (this bug was existing before my patch in r6814)

comment:19 Changed 6 years ago by Balaitous

I propose this:

  • by default nodes are evenly distributed
  • to prevent evenly distribution of a node, user have to explicitly select it
    /**
     * Perform AlignInCircle action.
     * 
     * A fixed node is a node for which it is forbidden to change the angle relative to center of the circle.
     * All other nodes are uniformly distributed.
     * 
     * Case 1: One unclosed way.
     * --> allow action, and align selected way nodes
     * If nodes contained by this way are selected, there are fix.
     * If nodes outside from the way are selected there are ignored.
     * 
     * Case 2: One or more ways are selected and can be joined into a polygon
     * --> allow action, and align selected ways nodes
     * If 1 node outside of way is selected, it became center
     * If 1 node outside and 1 node inside are selected there define center and radius
     * If no outside node and 2 inside nodes are selected those 2 nodes define diameter
     * In all other cases outside nodes are ignored
     * In all cases, selected nodes are fix, nodes with more than one referrers are fix
     * (first referrer is the selected way)
     * 
     * Case 3: Only nodes are selected
     * --> Align these nodes, all are fix
     */

This is implemented in bug8431_2.patch

Changed 6 years ago by Balaitous

Attachment: bug8431_2.patch added

since r6920

comment:20 in reply to:  19 Changed 6 years ago by skyper

Replying to Balaitous:

I do not really understand Case 1 and 3:

about 1.:

  • How should this work ? What does it have to do with circle
  • In code you use an temporally closed way but I can not think of many use-cases where an unclosed way should be a circle.
    • I have use-cases for align nodes in curve and the curve could be a circle but this is out of scope of "align nodes in circle".
    • I believe that start and end node need to be fixed to make some sense as you would be able to draw/align parts of a circle.

about 3.:

  • Can be dropped as all selected nodes are fixed anyway. We need at least one way or above is also true for this case but without start/end node.
  • I have several cases where I need to have some nodes of a way to form a circle but leave the rest in place or even keep the rest of the geometry in shape. So far I use some temporary construction ways to get to the result. Could this be done with case 3 ?
Last edited 6 years ago by skyper (previous) (diff)

comment:21 Changed 6 years ago by Balaitous

About 3:

  • It's exactly what JOSM already does (for example r6767)

About 1:

  • Temporally closed way is just a code convenience for the call to the collectNodesAnticlockwise function
  • first and last node are fix
                fixNodes.add(w.firstNode());
                fixNodes.add(w.lastNode());
    
  • It can produce nice half circle, but to be fully useful, need also #7423
  • There are use case for this, see attach file.

Changed 6 years ago by Balaitous

Attachment: use_case_of_half_circle.jpg added

comment:22 in reply to:  21 Changed 6 years ago by skyper

Replying to Balaitous:

About 3:

  • It's exactly what JOSM already does (for example r6767)

Thanks, I know why I always use an additional way: You are able have some fixed nodes.

About 1:

  • Temporally closed way is just a code convenience for the call to the collectNodesAnticlockwise function
  • first and last node are fix
                fixNodes.add(w.firstNode());
                fixNodes.add(w.lastNode());
    
  • It can produce nice half circle, but to be fully useful, need also #7423
  • There are use case for this, see attach file.

Ok, I have overread that part. Thanks again.

comment:23 Changed 6 years ago by skyper

Oh forgot. For case 1:

  • Why not allow to have several connected ways as long as the are in on row ?

comment:24 Changed 6 years ago by skyper

By the way, do you know the circle arc function from utilsplugin2 ?. I like it a lot but it is buggy and little orphaned.

Last edited 6 years ago by skyper (previous) (diff)

comment:25 Changed 6 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 6933/josm:

fix #8431 - improve "align in circle" action (patch by Balaitous):

  • by default nodes are evenly distributed
  • to prevent evenly distribution of a node, user have to explicitly select it

comment:26 Changed 6 years ago by Don-vip

thanks for the patch! this is really great :)

Changed 6 years ago by naoliv

Attachment: circle-uneven.png added

comment:27 Changed 6 years ago by naoliv

(if I need to open a new bug or if I am doing something wrong, please, tell me)

I have createcircle.nodecount=16 set in JOSM and with this new patch, creating an open triangle with 3 nodes (one way with 2 segments) and then pressing Shift+O I get a circle with 19 nodes that isn't evenly distributed:


Last edited 6 years ago by naoliv (previous) (diff)

comment:28 Changed 6 years ago by stoecker

Resolution: fixed
Status: closedreopened

comment:29 in reply to:  27 Changed 6 years ago by skyper

Replying to naoliv:

(if I need to open a new bug or if I am doing something wrong, please, tell me)

I have createcircle.nodecount=16 set in JOSM and with this new patch, creating an open triangle with 3 nodes (one way with 2 segments) and then pressing Shift+O I get a circle with 19 nodes that isn't evenly distributed:

This is an rather old problem and has nothing to do with the last changes. See #5922.

comment:30 in reply to:  23 Changed 6 years ago by Balaitous

Replying to skyper:

  • Why not allow to have several connected ways as long as the are in on row ?

It should be a good improvement. In France we have a lot of roundabout (the most important density around the word! ) some of them are broken into small arc to describe bus line.

By the way, do you know the circle arc function from utilsplugin2 ?. I like it a lot but it is buggy and little orphaned.

I don't know. I will have a look.

comment:31 in reply to:  26 ; Changed 6 years ago by Balaitous

Replying to Don-vip:

thanks for the patch! this is really great :)

If you think by contribution are useful, take a look at #7423. I correct a problem with circle center.

comment:32 in reply to:  31 ; Changed 6 years ago by Don-vip

Replying to Balaitous:

If you think by contribution are useful, take a look at #7423. I correct a problem with circle center.

Thanks, I did miss this one. I am considering it for inclusion in this release, see my remarks there.
Can you also take a look to naoliv and skyper comments about the createcircle.nodecount stuff? :)

comment:33 Changed 6 years ago by skyper

Summary: [patch] "Create circle" changes way direction"Create circle" changes way direction

comment:34 Changed 6 years ago by Don-vip

Resolution: fixed
Status: reopenedclosed

This problem is fixed, please continue discussion on #5922.

comment:35 in reply to:  32 ; Changed 6 years ago by Balaitous

Replying to Don-vip:

Can you also take a look to naoliv and skyper comments about the createcircle.nodecount stuff? :)

This is done.

comment:36 Changed 6 years ago by Don-vip

In 6942/josm:

fix #5922, see #8431 - value of createcircle.nodecount not always applied (patch by Balaitous)

comment:37 in reply to:  35 Changed 6 years ago by Don-vip

Replying to Balaitous:

This is done.

Thank you so much! You're doing a great job :)

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.