Modify

Opened 15 months ago

Last modified 8 months ago

#22799 new enhancement

[WIP patch] Smarter DistributeAction action for single node

Reported by: gaben Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: distribute nodes Cc:

Description

Problem

Imagine the following scenario:

  1. There is a way with a few nodes (o is a node):
    o---------o---o---o
    1         2   3   4
    
  2. Let's say the second node should be aligned relative to the first and third nodes:
    o------o------o---o
    1      2      3   4
    

Currently the only way for doing by selecting nodes 1, 2 and 3, then using the Distribute Nodes Action.

Proposal

With the new method (source code calls it algorithm), only the second node needs to be selected JOSM will figure out if possible, then act accordingly.

The patch is in testing, looks good for now. Probably I'll need help to make it unit testable.

Please provide any additional information below. Attach a screenshot if possible.

I may already created a ticket for it a few years back, couldn't find it now.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2023-03-05 17:42:37 +0100 (Sun, 05 Mar 2023)
Build-Date:2023-03-06 02:30:59
Revision:18682
Relative:URL: ^/trunk

Attachments (3)

josm22799_smart_distribute_test_data.osm (11.9 KB ) - added by gaben 15 months ago.
josm22799_smart_distribute.patch (4.9 KB ) - added by gaben 15 months ago.
unit test needed
josm22799_smart_distribute_v2.patch (5.0 KB ) - added by gaben 12 months ago.
add HTML notification formatting

Download all attachments as: .zip

Change History (8)

by gaben, 15 months ago

unit test needed

comment:1 by gaben, 12 months ago

Just noticed the notification description is not precise enough, should be edited. (Kind of a note for myself.)

by gaben, 12 months ago

add HTML notification formatting

comment:2 by gaben, 8 months ago

Summary: [WIP patch] Smarter Distribute Nodes action[patch] Smarter DistributeAction action for single node

Crated a PR on GH: pull/129.

Just realized that DistributeAction can cause overlapping way issues by design. That's the only reason I marked it as a draft, otherwise ready for merge.

To reproduce, create a zigzag way with 4 nodes, where the two inner nodes should be the furthest apart.

comment:3 by taylor.smock, 8 months ago

Another way to create an overlapping way issue is via the same zigzag issue you pointed out in comment:2, except you select one of the inner nodes instead of all the nodes. I was going to ask why you weren't using cmds = distributeWay(nodes.iterator().next().getParentWays(), nodes);, but that looks like it just performs the action on the entire way.

comment:4 by taylor.smock, 8 months ago

A minor suggestion:

We can split out a good chunk of the distributeNodes method out into its own method, e.g.

    private static Collection<Command> distributeNodes(INode nodea, INode nodeb, Collection<Node> nodes) {
        // Find out co-ords of A and B

and call that from the original distributeNodes with distributeNodes(nodea, nodeb, nodes) along with calling distributeNodes(neighbours.get(0), neighbours.get(1), new ArrayList<>(Collections.singleton(nodeToDistribute))); from distributeNode.

in reply to:  4 comment:5 by gaben, 8 months ago

Summary: [patch] Smarter DistributeAction action for single node[WIP patch] Smarter DistributeAction action for single node

I have a new idea to work around the overlapping way issue.

What if I:

  1. split the way by neighbour nodes (we have 2 or 3 ways at this point)
  2. apply the distributeWay() function to the new way (which has three nodes)
  3. merge the ways.

Replying to taylor.smock:

A minor suggestion:

We can split out a good chunk of the distributeNodes method out into its own method

Usually, I intentionally do not make changes to existing code or data flow because

  1. I don't want to break things
  2. I only have JOSM core checked out
  3. it's usually rewritten after the review anyway

If you already brought up structural changes/simplifications, I think there should be some public utility methods for such primitive calculations. And yes, it looks like we have a duplicate in org.openstreetmap.josm.actions.AlignInLineAction#nodePairFurthestApart (you'll find it as "Appart", there is a typo in the name).

In summary, with these proposals we could simplify two classes and fix the ticket.

I might open a task ticket with the duplicates, or good utility method candidates I find along coding.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to gaben.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.