Modify

Opened 7 years ago

Closed 5 years ago

#7991 closed defect (fixed)

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

Reported by: skyper Owned by: team
Priority: normal Milestone: 14.06
Component: Core Version: latest
Keywords: extruder Cc:

Description (last modified by 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.

screenshot

Have a look at #7723 for background.

r5471

Attachments (23)

extruder_smaller.png (22.9 KB) - added by skyper 7 years ago.
screenshot
extruder_smaller_example.osm (3.4 KB) - added by skyper 7 years ago.
osm file
7991-moving.png (18.6 KB) - added by AlfonZ 6 years ago.
workaround 1
7991-preserveAngle.png (24.8 KB) - added by AlfonZ 6 years ago.
workaround 2
7991-ComplexCases.png (32.7 KB) - added by AlfonZ 6 years ago.
7991-MoveBothNodes-mockup.png (5.1 KB) - added by AlfonZ 6 years ago.
AutoAddIntersections.py (2.5 KB) - added by AlfonZ 6 years ago.
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.
7991-DualAlign.patch (34.4 KB) - added by AlfonZ 6 years ago.
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
dualalign.png (371 bytes) - added by AlfonZ 6 years ago.
Menuitem's icon, accompanies 7991-DualAlign.patch. Goes to images/mapmode/extrude/.
7991-DualAlign-Ctrl-compare.png (6.5 KB) - added by AlfonZ 6 years ago.
Left: Moving segment, Dual alignment disabled Right Moving segment, Dual alignment enabled
7991-DualAlign.r6168.patch (34.4 KB) - added by AlfonZ 6 years ago.
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
7991-DualAlign.r6252.patch (34.4 KB) - added by AlfonZ 6 years ago.
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
7991-AnnotatedNodes.png (3.9 KB) - added by AlfonZ 6 years ago.
7991-DualAlign.r6252.2.patch (34.4 KB) - added by AlfonZ 6 years ago.
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.
7991-DualAlign.r7206.patch (34.2 KB) - added by AlfonZ 5 years ago.
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.
dualalign_badform.png (3.4 KB) - added by akks 5 years ago.
dualalign_stophere.png (1.7 KB) - added by akks 5 years ago.
DualAlign_example1.png (12.1 KB) - added by AlfonZ 5 years ago.
DualAlign_example1_merged.png (3.7 KB) - added by AlfonZ 5 years ago.
extrude_sample_autofix.png (8.6 KB) - added by akks 5 years ago.
DualAlignLimitStatus.patch (2.0 KB) - added by AlfonZ 5 years ago.
Add indication when the segment collapsing due to extrusion limit is being performed.
7991-test-TwoNodes.osm (957 bytes) - added by AlfonZ 5 years ago.
7991-DualAlignFixes.patch (2.3 KB) - added by AlfonZ 5 years ago.
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.

Download all attachments as: .zip

Change History (66)

Changed 7 years ago by skyper

Attachment: extruder_smaller.png added

screenshot

Changed 7 years ago by skyper

osm file

comment:1 Changed 7 years ago by skyper

Description: modified (diff)

comment:2 Changed 7 years ago by 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.

comment:3 Changed 6 years ago by 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).

Changed 6 years ago by AlfonZ

Attachment: 7991-moving.png added

workaround 1

Changed 6 years ago by AlfonZ

Attachment: 7991-preserveAngle.png added

workaround 2

comment:4 Changed 6 years ago by 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.

workaround 1

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.

workaround 2

comment:5 in reply to:  4 Changed 6 years ago by 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.

Changed 6 years ago by AlfonZ

Attachment: 7991-ComplexCases.png added

comment:6 Changed 6 years ago by 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.)


comment:7 in reply to:  6 ; Changed 6 years ago by 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.

comment:8 in reply to:  7 ; Changed 6 years ago by 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.

Changed 6 years ago by AlfonZ

comment:9 Changed 6 years ago by 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?

comment:10 in reply to:  8 ; Changed 6 years ago by 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.

comment:11 in reply to:  10 ; Changed 6 years ago by 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 :)

comment:12 in reply to:  11 ; Changed 6 years ago by 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.

Changed 6 years ago by AlfonZ

Attachment: AutoAddIntersections.py added

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.

comment:13 in reply to:  10 ; Changed 6 years ago by 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.

comment:14 in reply to:  13 Changed 6 years ago by 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

Changed 6 years ago by AlfonZ

Attachment: 7991-DualAlign.patch added

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

Changed 6 years ago by AlfonZ

Attachment: dualalign.png added

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

Changed 6 years ago by AlfonZ

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

comment:15 in reply to:  12 Changed 6 years ago by AlfonZ

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

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)

Left: Moving segment, //Dual alignment// disabled[[br]] Right Moving segment, //Dual alignment// enabled

comment:16 Changed 6 years ago by 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...)

Changed 6 years ago by AlfonZ

Attachment: 7991-DualAlign.r6168.patch added

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

comment:17 Changed 6 years ago by 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 !

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

comment:18 Changed 6 years ago by 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?

Changed 6 years ago by AlfonZ

Attachment: 7991-DualAlign.r6252.patch added

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

Changed 6 years ago by AlfonZ

Attachment: 7991-AnnotatedNodes.png added

comment:19 in reply to:  18 Changed 6 years ago by 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.

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

Changed 6 years ago by 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.

Changed 5 years ago by AlfonZ

Attachment: 7991-DualAlign.r7206.patch added

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.

comment:20 Changed 5 years ago by AlfonZ

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

comment:21 Changed 5 years ago by 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?

comment:22 Changed 5 years ago by akks

In 7216/josm:

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

Changed 5 years ago by akks

Attachment: dualalign_badform.png added

Changed 5 years ago by akks

Attachment: dualalign_stophere.png added

comment:23 Changed 5 years ago by 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:

comment:24 Changed 5 years ago by Don-vip

Milestone: 14.06

comment:25 Changed 5 years ago by akks

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

comment:26 Changed 5 years ago by akks

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

Changed 5 years ago by AlfonZ

Attachment: DualAlign_example1.png added

Changed 5 years ago by AlfonZ

comment:27 in reply to:  23 Changed 5 years ago by 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.

comment:28 Changed 5 years ago by 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...

Last edited 5 years ago by akks (previous) (diff)

comment:29 Changed 5 years ago by 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

comment:30 Changed 5 years ago by 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?

Last edited 5 years ago by akks (previous) (diff)

Changed 5 years ago by akks

Attachment: extrude_sample_autofix.png added

comment:31 in reply to:  30 Changed 5 years ago by 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.

Changed 5 years ago by AlfonZ

Attachment: DualAlignLimitStatus.patch added

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

comment:32 Changed 5 years ago by 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)?

comment:33 Changed 5 years ago by akks

In 7226/josm:

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

comment:34 Changed 5 years ago by 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.

Last edited 5 years ago by akks (previous) (diff)

Changed 5 years ago by AlfonZ

Attachment: 7991-test-TwoNodes.osm added

comment:35 Changed 5 years ago by AlfonZ

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

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

     
    250250        if (mode == Mode.select) {
    251251            rv = new StringBuilder(tr("Drag a way segment to make a rectangle. Ctrl-drag to move a segment along its normal, " +
    252252                "Alt-drag to create a new rectangle, double click to add a new node."));
    253             if (dualAlignEnabled) {
     253            if (dualAlignEnabled)
    254254                rv.append(" ").append(tr("Dual alignment active."));
    255                 if (dualAlignSegmentCollapsed) {
    256                     rv.append(" ").append(tr("Segment collapsed due to its direction reversing."));
    257                 }
    258             }
    259255        } else {
    260256            if (mode == Mode.translate)
    261257                rv = new StringBuilder(tr("Move a segment along its normal, then release the mouse button."));
     
    269265                Main.warn("Extrude: unknown mode " + mode);
    270266                rv = new StringBuilder();
    271267            }
    272             if (dualAlignActive)
     268            if (dualAlignActive) {
    273269                rv.append(" ").append(tr("Dual alignment active."));
     270                if (dualAlignSegmentCollapsed) {
     271                    rv.append(" ").append(tr("Segment collapsed due to its direction reversing."));
     272                }
     273            }
    274274        }
    275275        return rv.toString();
    276276    }

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.

comment:36 Changed 5 years ago by 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?

comment:37 Changed 5 years ago by akks

In 7232/josm:

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

comment:38 Changed 5 years ago by 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)

Changed 5 years ago by AlfonZ

Attachment: 7991-DualAlignFixes.patch added

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.

comment:39 in reply to:  38 Changed 5 years ago by 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.

comment:40 Changed 5 years ago by akks

In 7247/josm:

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

comment:41 Changed 5 years ago by 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.

Last edited 5 years ago by akks (previous) (diff)

comment:42 Changed 5 years ago by Don-vip

is there still something to do on this ticket?

comment:43 Changed 5 years ago by Don-vip

Resolution: fixed
Status: newclosed

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

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.