[patch] extruder: smallering non-rectified object can lead to unwanted, self-intersecting objects.

[skyper]

Using extruder to smaller non-rectified objects can lead to unwanted and self-intersecting objects. This methode should be smarter to produce wanted results and should never produce self-intersecting objects.

Seems to me that I can not produce a valid result at all in some situations.

Have a look at #7723 for background.

r5471

[skyper]

screenshot

[skyper]

osm file

[skyper]

Somehow we need information about the object's outline to avoid these self-intersections. E.g. if you split the south wall in three pieces and move only the middle part the method is working right now but for the left/right piece it won't.

[akks]

It is also very easy to create self-intersecting way by Parallel Way. This may be fixed toether, but it can be very hard (imagine segments intersecting saw-tooth or even spiral borders).

[AlfonZ]

workaround 1

[AlfonZ]

workaround 2

[AlfonZ]

It seems to me, that what you are trying to do is move the segment, as opposed to extrude it (extruding to me implies creating new node(s) and leaving at least one of the segment's nodes untouched).
Could you try following:
Move the segment (using Ctrl) along neighboring segment, then move node C.

If preserving the angle ABC is important, then more elaborate steps might be needed:
Extrude the segment along neighboring segment, add new node where segments intersect, merge three nodes into one C.

[skyper]

Replying to AlfonZ:

It seems to me, that what you are trying to do is move the segment, as opposed to extrude it (extruding to me implies creating new node(s) and leaving at least one of the segment's nodes untouched).

No, I should use Shift to get extra nodes. Considering nodes which are only part of one way they should be moved if possible.

If preserving the angle ABC is important, then more elaborate steps might be needed:
Extrude the segment along neighboring segment, add new node where segments intersect, merge three nodes into one C.

I know how to work around it. Thanks for your explanation once more, but my question are:

• Why is extruder producing ways validator is warning about ?
  • Does not make sense.
• Is it not possible to make eXtruder smarter ?
• Why bother the user if it would be possible to automatically solve this problem ?
  • Instead user has to switch modes to get to a proper result.

[AlfonZ]

The suggested optimization raises questions what is the desired behavior in more complex cases.

Also, it is possible to create intersecting segments by using other modes (move segment, move node) as well. Should these be cleaned up as well? If so, what key modifiers they should be assigned to?
It has to be possible to use current behavior, so users can create interim intersecting geometry they intend to refine later. (For basic extruding that would be cleanup by default and no cleanup on Shift.)

[skyper]

Replying to AlfonZ:

The suggested optimization raises questions what is the desired behavior in more complex cases.

Also, it is possible to create intersecting segments by using other modes (move segment, move node) as well. Should these be cleaned up as well? If so, what key modifiers they should be assigned to?

+1

Why not ? But first, let's try it with extruder mode.

It has to be possible to use current behavior, so users can create interim intersecting geometry they intend to refine later. (For basic extruding that would be cleanup by default and no cleanup on Shift.)

+5

Maybe even add new nodes to the intersections that we will have self-intersecting ways but no overlaps.

[AlfonZ]

My point of view is that Extrusion is primarily creation tool, which creates new, possibly messy, geometry.
Then there are other tools, designed to help user clean up that mess (move, merge, join node/areas, etc.), each in it's own particular way.

I am not saying Extrusion should be as messy as possible and let user deal with that mess.
It's just I see too many options how it can be cleaned up (the one suggested above being one of them and even creating new options) and it's not yet clear to me how to solve it in a consistent, unambiguous and user-friendly manner.

Replying to skyper:

Replying to AlfonZ:

It has to be possible to use current behavior, so users can create interim intersecting geometry they intend to refine later. (For basic extruding that would be cleanup by default and no cleanup on Shift.)

+5

Maybe even add new nodes to the intersections that we will have self-intersecting ways but no overlaps.

This is exactly what utilsplugin2's Add nodes at intersections does. The user may or may not want to do it. Putting it into Extrude would require yet another modifier and complicate it with very little gain.

[AlfonZ]

I have an alternative idea that seems feasible.
Instead of moving just one node along neighboring segment (and then do cleanup), move both segment's nodes.

This behavior would be controlled similar to Draw mode's Angle snapping - toggled by shortcut and entry in the Edit menu.
However, it would still be possible to create self-intersecting way when extruding too far.

Would this help?

[skyper]

Replying to AlfonZ:

Replying to skyper:

Maybe even add new nodes to the intersections that we will have self-intersecting ways but no overlaps.

This is exactly what utilsplugin2's Add nodes at intersections does. The user may or may not want to do it. Putting it into Extrude would require yet another modifier and complicate it with very little gain.

So with utilsplugin2 installed I only need an option to call this function automatically by extruder.

[akks]

Replying to skyper:

So with utilsplugin2 installed I only need an option to call this function automatically by extruder.

We do not need Utilsplugin2 for that - Geometry.addNodesOnINtersections is alread in core :)

[skyper]

Replying to AlfonZ:

I have an alternative idea that seems feasible.
Instead of moving just one node along neighboring segment (and then do cleanup), move both segment's nodes.

nice.

This behavior would be controlled similar to Draw mode's Angle snapping - toggled by shortcut and entry in the Edit menu.
However, it would still be possible to create self-intersecting way when extruding too far.

Would this help?

+1
Definitely an nice option to have.

Replying to akks:

Replying to skyper:

So with utilsplugin2 installed I only need an option to call this function automatically by extruder.

We do not need Utilsplugin2 for that - Geometry.addNodesOnINtersections is alread in core :)

Then this would be another option, either set through preferences or shortcut/modifier.

[AlfonZ]

Python script for ​scripting plugin, also requires UtilsPlugin2.
Adds command queue listener that will perform utilsplugin2's AddNodesAtIntersections command after encountering Extrude command.
Run once per JOSM session.

[AlfonZ]

Replying to skyper:

So with utilsplugin2 installed I only need an option to call this function automatically by extruder.

I have attached AutoAddIntersections.py​ - script for ​scripting plugin (using python engine).
It will check each new command and if it's Extrude, it will perform utilsplugin2's Add nodes at intersections.
While it's true that using core's Geometry.addIntersections would be sufficient, utilsplugin2's Add nodes at intersections wraps it with command stack support that I did not feel like duplicating.

[skyper]

Replying to AlfonZ:

Replying to skyper:

So with utilsplugin2 installed I only need an option to call this function automatically by extruder.

I have attached AutoAddIntersections.py​ - script for ​scripting plugin (using python engine).
It will check each new command and if it's Extrude, it will perform utilsplugin2's Add nodes at intersections.
While it's true that using core's Geometry.addIntersections would be sufficient, utilsplugin2's Add nodes at intersections wraps it with command stack support that I did not feel like duplicating.

Thanks, did link it under wiki:Help/Plugin/Scripting

[AlfonZ]

new checkbox menuitem (in Edit menu)
new subclass DualAlignChangeAction that toggles the mode
key event handling copied from DrawAction
MoveCommands: when using Ctrl modifier with dual alignment disabled, only moveCommand is used; with dual alignment enabled, moveCommand and new moveCommand2 is used, due to two nodes moving in different directions, commands are then grouped into SequenceCommand
javadoc updates

[AlfonZ]

Menuitem's icon, accompanies 7991-DualAlign.patch​. Goes to images/mapmode/extrude/.

[AlfonZ]

Left: Moving segment, Dual alignment disabled
Right Moving segment, Dual alignment enabled

[AlfonZ]

Replying to skyper:

Replying to AlfonZ:

I have an alternative idea that seems feasible.
Instead of moving just one node along neighboring segment (and then do cleanup), move both segment's nodes.

nice.

This behavior would be controlled similar to Draw mode's Angle snapping - toggled by shortcut and entry in the Edit menu.
However, it would still be possible to create self-intersecting way when extruding too far.

Would this help?

+1
Definitely an nice option to have.

I've attached 7991-DualAlign.patch​.

I called this feature Dual alignment. (Is there a better name for it?)

It can be toggled in Edit menu (while in Extrude mode) or by keyboard shortcut (X).
As to avoid the return of #7751 look-alike, default shortcut can be disabled via extrude.dualalign.toggleOnRepeatedX and new one set via Mode: Extrude Dual alignment shortcut.

When enabled, applies to basic extrude, Shift modifier (always create nodes), Alt (create new way) and Ctrl (move segment), provided the extruded segment has non-parallel neighbors.
There is only one possible direction of extruding, end points of extruded segment are aligned to its neighboring segments.
Status is indicated in the status line and visually (two reference segments drawn instead of one).

Other things of note:

• The menuitem's icon may not be fitting. Feel free to change it.
• Difference in behaviour when compared do Draw mode's Angle snapping:
  • Angle snapping menuitem is enabled after startup, even in Select mode, gets disabled after exiting Draw mode. Dual alignment menuitem is disabled by default.
  • Angle snapping is disabled when entering Draw mode, Dual alignment's status is unchanged from last Extrude.
• When moving segment without Dual alignment, there is helper line indicating direction of move. With Dual alignment enabled, this line is not shown (two reference segments are shown instead)

[akks]

The patch is good, but we have something to do with these timer-based AWTListeners
(I introduced it in AddAction and there are problems - we need repeated key press detection for Select Action etc but can not put this horrible code everywhere...)

[AlfonZ]

patch updated to r6168
new checkbox menuitem (in Edit menu)
new subclass DualAlignChangeAction that toggles the mode
key event handling copied from DrawAction
MoveCommands: when using Ctrl modifier with dual alignment disabled, only moveCommand is used; with dual alignment enabled, moveCommand and new moveCommand2 is used, due to two nodes moving in different directions, commands are then grouped into SequenceCommand
javadoc updates

[skyper]

@AlfonZ:
Trac does not update the modified time with attachments. Please always add an separate comment (which I did with this comment). - Thank

EDT: Nor are there any emails send !

[stoecker]

@AlfonZ:

Does not apply fully anymore. Can you update it and describe very short why it is necessary and what negative effects it could have?

[AlfonZ]

patch updated to r6252
new checkbox menuitem (in Edit menu)
new subclass DualAlignChangeAction that toggles the mode
key event handling copied from DrawAction
MoveCommands: when using Ctrl modifier with dual alignment disabled, only moveCommand is used; with dual alignment enabled, moveCommand and new moveCommand2 is used, due to two nodes moving in different directions, commands are then grouped into SequenceCommand
javadoc updates

[AlfonZ]

Replying to stoecker:

Does not apply fully anymore. Can you update it and describe very short why it is necessary and what negative effects it could have?

r6252 got in the way. Patch updated​.

The purpose of the patch is to make one case of extruding easier - when user wants to preserve both angles of extruded segment (ABC and BCD on picture below).
Currently, it is possible to preserve only one of the angles. To achieve desired result, one has to employ more elaborate workflow, some examples mentioned in 4:ticket:7991.

From user point of view, I don't really see any negatives.
From technical point of view, there is code duplication of timer-based key event handling with Draw Action (second instance) and its usage itself, as mentioned by akks.

[AlfonZ]

patch updated to r6252
new checkbox menuitem (in Edit menu)
new subclass DualAlignChangeAction that toggles the mode
key event handling copied from DrawAction
MoveCommands: when using Ctrl modifier with dual alignment disabled, only moveCommand is used; with dual alignment enabled, moveCommand and new moveCommand2 is used, due to two nodes moving in different directions, commands are then grouped into SequenceCommand
javadoc updates
Needs dualalign.png​.

[AlfonZ]

patch updated to r7206
new checkbox menuitem (in Edit menu)
new subclass DualAlignChangeAction that toggles the mode
key event handling copied from DrawAction
MoveCommands: when using Ctrl modifier with dual alignment disabled, only moveCommand is used; with dual alignment enabled, moveCommand and new moveCommand2 is used, due to two nodes moving in different directions, commands are then grouped into SequenceCommand
javadoc updates
replaced string concatenation by StringBuilder - see r6289
loose coupling - see r6317
Needs dualalign.png​.

[AlfonZ]

Updated patch (7991-DualAlign.r7206.patch​) to accomodate (conflicting) changes done in r6336 and r6792.

[akks]

Yes, this functionalty is very good. The patch really deserves to be included and have been ignored for too long already.

@team: Can someone place this AWTListener stuff in MapMode or MapView to avi=oid duplication and support buuton holding and repeated pressing for all the tools?

[akks]

In 7216/josm:

Add Dual Align mode for extruder (patch by AlfonZ, see #7991)

[akks]

@AlfonZ: Thank you for the patch! Sorry for delaying it for so long.

ToDo before closing the ticket:

• use one AWTListener instance for all advanced button-press-detection (I'll have to do it myself, together with lasso submode switching #3910)
• try to avoid self-intersections with neighboring segments:

, better stop here:

[akks]

I am committing the big patch eliminating most of duplicated AWTListeners used to detect Alt-Shift-Ctrl changes and also button holding.

[akks]

AWTListener removed, see #10104
It would be very good if someone can check that the behavior did not change on Linux and Mac

[AlfonZ]

Replying to akks:

• try to avoid self-intersections with neighboring segments:

, better stop here:

I'm not sure what you mean.
In this particular example, the end result is non-intersecting segment:
If your intended result is , then IMHO extrude's Dual alignment is the wrong tool for the job. I'd use extrude's Move node mode followed by utilsplugin2's Add nodes at intersections, then Merge nodes.

[akks]

Sorry, wording was not exact.

We could try to avoid extruded segment direction reversion (BC new direction is opposite to its initial direction).

The result can of course be obtained by suitable instruments, but users can start complaining about th "bad" new instrument when they accidentally make something like this. Stopping the movement of segment when it is going to reverse should not be very complex...

[akks]

In 7223/josm:

Extrude mode: do not allow reversing the moved segment direction to avoid strange shapes, see #7991
can be disabled by extrude.dualalign.keep-segment-direction=false

[akks]

I tried to add the extrusion limit, now it does not allow changing the direction of extruded segment by default.
If the dragged segment collapse to point, then the nodes are merged (by MergeNodesActions methods).

I did not modify ctrl-drag behavior. Is there any planned dirfference in this mode when DualAlign is active?

[AlfonZ]

Replying to akks:

I tried to add the extrusion limit, now it does not allow changing the direction of extruded segment by default.

My personal preference would be that this is disabled by default.

Is it really needed for keepSegmentDirection to be final and set in the constructor as opposed to in enterMode()? I don't particularly like that changing its value effectively requires restart.

If the dragged segment collapse to point, then the nodes are merged (by MergeNodesActions methods).

I think there should be some indication that the limiting takes place, at least something in the status like DualAlignLimitStatus.patch​.

Having nodes that require conflict resolution when merging (e.g. example above), after cancelling the conflict resolution window an exception shows up:

java.lang.IllegalArgumentException: Parameter 'c' must not be null
	at org.openstreetmap.josm.tools.CheckParameterUtil.ensureParameterNotNull(CheckParameterUtil.java:79)
	at org.openstreetmap.josm.data.UndoRedoHandler.addNoRedraw(UndoRedoHandler.java:42)
	at org.openstreetmap.josm.data.UndoRedoHandler.add(UndoRedoHandler.java:69)
	at org.openstreetmap.josm.actions.mapmode.ExtrudeAction.performExtrusion(ExtrudeAction.java:677)
	at org.openstreetmap.josm.actions.mapmode.ExtrudeAction.mouseReleased(ExtrudeAction.java:519)
...

Alt+DualAlign and Ctrl+DualAlign don't perform nodes merging, resulting in nodes at the same position.

I did not modify ctrl-drag behavior. Is there any planned difference in this mode when DualAlign is active?

Not that I know of. All modifiers (Shift, Alt, Ctrl) are meaningful with DualAlign though. In particular, the difference between normal behavior and Ctrl is when segment's nodes are shared with other way, in the first case new node is created, in the latter case the shared node is moved, thus changing the other way presumably in a desirable way.

[AlfonZ]

Add indication when the segment collapsing due to extrusion limit is being performed.

[akks]

Thank you for notes, trying to fix option-reader, exception and ctrl-collapsing.

The patch and idea is nice, but I doubt that many users look to status line. Maybe the cursor changes would be more obvious (or together with status line)?

[akks]

In 7226/josm:

see #7991:patch by AlfonZ mod. Show and correctly handle segment collapsing in X mode

[akks]

I did not invent a better method to show collapsing than in status line. Exceptions should be gone and Ctrl-drag can merge nodes too.

[AlfonZ]

Status line indication while collapsing is showing in select mode instead of extrude mode:

src/org/openstreetmap/josm/actions/mapmode/ExtrudeAction.java

@@ -250 +250 @@
         if (mode == Mode.select) {
             rv = new StringBuilder(tr("Drag a way segment to make a rectangle. Ctrl-drag to move a segment along its normal, " +
                 "Alt-drag to create a new rectangle, double click to add a new node."));
-            if (dualAlignEnabled) {
+            if (dualAlignEnabled)
                 rv.append(" ").append(tr("Dual alignment active."));
-                if (dualAlignSegmentCollapsed) {
-                    rv.append(" ").append(tr("Segment collapsed due to its direction reversing."));
-                }
-            }
         } else {
             if (mode == Mode.translate)
                 rv = new StringBuilder(tr("Move a segment along its normal, then release the mouse button."));
@@ -269 +265 @@
                 Main.warn("Extrude: unknown mode " + mode);
                 rv = new StringBuilder();
             }
-            if (dualAlignActive)
+            if (dualAlignActive) {
                 rv.append(" ").append(tr("Dual alignment active."));
+                if (dualAlignSegmentCollapsed) {
+                    rv.append(" ").append(tr("Segment collapsed due to its direction reversing."));
+                }
+            }
         }
         return rv.toString();
     }

Ctrl+click on a way or a node without moving the mouse, exception occurs:

java.lang.NullPointerException
	at org.openstreetmap.josm.actions.mapmode.ExtrudeAction.joinNodesIfCollapsed(ExtrudeAction.java:689)
	at org.openstreetmap.josm.actions.mapmode.ExtrudeAction.mouseReleased(ExtrudeAction.java:536)
	at java.awt.AWTEventMulticaster.mouseReleased(Unknown Source)
...

DualAlign extrude the way CD from 7991-test-TwoNodes.osm​, an exception occurs:

java.lang.IllegalArgumentException: Parameter 'p4' must not be null
	at org.openstreetmap.josm.tools.CheckParameterUtil.ensureParameterNotNull(CheckParameterUtil.java:79)
	at org.openstreetmap.josm.tools.CheckParameterUtil.ensureValidCoordinates(CheckParameterUtil.java:55)
	at org.openstreetmap.josm.tools.Geometry.segmentsParallel(Geometry.java:350)
	at org.openstreetmap.josm.actions.mapmode.ExtrudeAction.performExtrusion(ExtrudeAction.java:646)
	at org.openstreetmap.josm.actions.mapmode.ExtrudeAction.mouseReleased(ExtrudeAction.java:531)
	at java.awt.AWTEventMulticaster.mouseReleased(Unknown Source)
...

Ctrl+click on the way CD from 7991-test-TwoNodes.osm​, an exception occurs:

java.lang.NullPointerException
	at org.openstreetmap.josm.data.coor.EastNorth.sub(EastNorth.java:102)
	at org.openstreetmap.josm.actions.mapmode.ExtrudeAction.mouseDragged(ExtrudeAction.java:473)
	at java.awt.AWTEventMulticaster.mouseDragged(Unknown Source)
...

Alt+DualAlign+Collapsing results in two nodes without merging.
Ctrl+move node performs superfluous merging of the one node.

[akks]

Sorry, problems with manual patch applying. Fixed the NPE, Alt+collapse and Ctrl-extra-merge.

In the example D and E nodes are coincident, so we'd better turn off dualalignment for them, yes?

[akks]

In 7232/josm:

see #7991: more fixes to X DualAlign mode (fix exceptions, Alt-drag can create triangles)

[akks]

This time it should be more secure.

@AlfonZ: Could you please check once more? Do you agree that for novices it is better to have the limitation enabled by default?
(preferences reading fixed)

[AlfonZ]

Setting DualAlignSegmentCollapsed to false at the beginning and the end of action, so status line displays correctly.
Alt+DualAlign+Collapse now doesn't even create the fourth node.

[AlfonZ]

Fixed small issues in 7991-DualAlignFixes.patch​
Setting DualAlignSegmentCollapsed to false at the beginning and the end of action, so status line displays correctly.
Alt+DualAlign+Collapse now doesn't even create the fourth node.

Replying to akks:

In the example D and E nodes are coincident, so we'd better turn off dualalignment for them, yes?

Yes, that's right.

Replying to akks:

@AlfonZ: Could you please check once more? Do you agree that for novices it is better to have the limitation enabled by default?

Unfortunately, I'm no novice :(. I hoped someone higher-up would chime in.
When disabled, the extrusion line moves according to the mouse cursor, which is logical/expected, but might create overlapping segments, which isn't novice-friendly.
When enabled, there are no overlapping segments, which is novice-friendly, but the mouse cursor-extrusion line discrepancy might seem unexpected.

[akks]

In 7247/josm:

see #7991: patch by AlfonZ - minor fix to segment collapsion in extrude mode

[akks]

I prefer to avoid new options being disabled by default - no one will ever turn them on (and novices will avoid dual align that creates "weird" shapes), but if it bothers experienced user, he can simply turn them off (there was such problems with angle snapping too - some people did not like helper lines etc.)

@all: I tried to indicate segment collapsing by cursor change or by appearing text but it looked stranger than it is now. If there is an ides how to show it, please post it here.

[Don-vip]

is there still something to do on this ticket?

[Don-vip]

Let's say it is fixed. If some issues remain, please open a new ticket.