Modify

Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#7239 closed enhancement (fixed)

[patch] Join node to way should operate on all selected nodes

Reported by: *Martin* Owned by: team
Priority: normal Milestone: 14.07
Component: Core Version: latest
Keywords: Cc:

Description

Join node to way should operate on all selected nodes. Please check my patch. Not sure if it is the best approach but it works and also obeys the undo buffer.

Attachments (2)

multijoin.diff (3.1 KB) - added by *Martin* 8 years ago.
the patch
multijoin3.diff (8.0 KB) - added by cymerio 5 years ago.
My implementation of the feature

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by *Martin*

Attachment: multijoin.diff added

the patch

comment:1 Changed 8 years ago by bastiK

Patch looks ugly, are you sure it has to be done like that?

comment:2 Changed 8 years ago by *Martin*

What's your problem with it? Do you have any better solution already? Next time I'll only report the issue and leave my ugly patches for my own. Good luck.

comment:3 Changed 8 years ago by bastiK

Sorry for the rude language, it wasn't my intention to offend you.

It's a weakness of the current undo/redo system, that you cannot build on previous commands and still keep it grouped in one SequenceCommand, it has to be done "in one go". So rather than calling executeCommand(), I would suggest to compile the final list of way nodes directly. Then, one ChangeCommand for each way will be enough.

comment:4 in reply to:  3 ; Changed 8 years ago by *Martin*

Replying to bastiK:

Sorry for the rude language, it wasn't my intention to offend you.

Offending my code offends me ;-)

It's a weakness of the current undo/redo system, that you cannot build on previous commands and still keep it grouped in one SequenceCommand, it has to be done "in one go".

This is exactly why I "hacked" (extended) SequenceCommand.

So rather than calling executeCommand(), I would suggest to compile the final list of way nodes directly. Then, one ChangeCommand for each way will be enough.

This was my first implementation. It was almost working, but the problem was that if there were multiple nodes joined to the same way segment, then their order was non-deterministic. I believe this is possible to solve too, but it is still much more difficult than my first "hacky" solution that emulates something like "transactions"...

I think it would be good to have something like a "transaction system", where it would be possible to submit changes in any phase of the process so that subsequent operations within same transaction would work on data modified by previous operation. Afterwards the transaction would be commited or rolled back. The commit would also represent one undo unit.

This could maybe make even more algorithms simpler because they could then drop the part that collects changes made by previous steps of the whole action to finally create consistent changes.

In any case I can still (try) to re-implement it to the way you suggest if this feature is desired (for me it is). What do you think?

comment:5 in reply to:  4 Changed 8 years ago by bastiK

Replying to martin.zdila@…:

Replying to bastiK:
This was my first implementation. It was almost working, but the problem was that if there were multiple nodes joined to the same way segment, then their order was non-deterministic. I believe this is possible to solve too, but it is still much more difficult than my first "hacky" solution that emulates something like "transactions"...

I think it would be good to have something like a "transaction system", where it would be possible to submit changes in any phase of the process so that subsequent operations within same transaction would work on data modified by previous operation. Afterwards the transaction would be commited or rolled back. The commit would also represent one undo unit.

This could maybe make even more algorithms simpler because they could then drop the part that collects changes made by previous steps of the whole action to finally create consistent changes.

In any case I can still (try) to re-implement it to the way you suggest if this feature is desired (for me it is). What do you think?

I'd say it would be an improvement and the feature is welcome. You can either do it "oldschool" or it might really be a good idea to extend the undo/redo system. In this case you probably have to keep in mind multithreading, so different "execution sequences" don't get interleaved in an unpredictable way.

comment:6 Changed 8 years ago by bastiK

Summary: [PATCH] Join node to way should operate on all selected nodes[PATCH (needs rework)] Join node to way should operate on all selected nodes

comment:7 Changed 7 years ago by stoecker

Summary: [PATCH (needs rework)] Join node to way should operate on all selected nodesJoin node to way should operate on all selected nodes

comment:8 Changed 7 years ago by Don-vip

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

Changed 5 years ago by cymerio

Attachment: multijoin3.diff added

My implementation of the feature

comment:9 Changed 5 years ago by cymerio

I've attached my implementation of the feature. I was in need of such feature and couldn't find any patch that applies with the current code base, so I implemented it in my own way.
It was my first time programming in Java, so it may contain mistakes. Please review.
It has some little inefficiencies, but works pretty fast for me.

It works the same as before for 1 node selected (including limiting the joining operation to selected ways).
For multiple nodes, it works joining the nodes all at once, not one after another. Consider the case when joining the first node modifies the shape of the way, then the second node is now so far from the way that can not snap to it. This patch doesn't show that problem: it computes first how it is going to join all the nodes, then at a later stage it makes all the modifications to the ways/nodes.

Note: this works really great to clean some types of geometry data, like the new Spanish Catastro datasource!

comment:10 Changed 5 years ago by skyper

Summary: Join node to way should operate on all selected nodes[patch] Join node to way should operate on all selected nodes

comment:11 Changed 5 years ago by bastiK

Resolution: fixed
Status: newclosed

In 7302/josm:

applied #7239 - Join node to way should operate on all selected nodes (patch by cymerio)

comment:12 Changed 5 years ago by bastiK

Great, thanks! I only did some minor cosmetic changes.

comment:13 Changed 5 years ago by Don-vip

Milestone: 14.07

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.