Modify

Opened 10 years ago

Closed 9 years ago

#9584 closed enhancement (needinfo)

[Patch in development] Support cardinal direction member roles in LinkedCellRenderer

Reported by: mvexel Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: Cc:

Description

This covers cardinal direction support in the relation editor, particularly in the LinkedCellRenderer.

Right now, only the 'forward' and 'backward' roles are considered when rendering the relation visualization in the LinkedCellRenderer. This patch expands the support to 'north' / 'south' and 'east' / 'west', member roles that are frequently used in the United States and other countries where the motorways / freeways are signposted with cardinal directions, for example: Interstate 80 East.

Attachments (4)

cardinaldirections.patch (29.2 KB) - added by mvexel 10 years ago.
9584_v2.patch (8.7 KB) - added by simon04 10 years ago.
I80_west_unconnected.png (52.5 KB) - added by simon04 10 years ago.
9584_v3.patch (8.0 KB) - added by mvexel 10 years ago.
new version based on comments.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 10 years ago by mvexel

Summary: Support cardinal direction member roles inSupport cardinal direction member roles in LinkedCellRenderer

comment:2 Changed 10 years ago by Don-vip

Summary: Support cardinal direction member roles in LinkedCellRenderer[Patch] Support cardinal direction member roles in LinkedCellRenderer

Can you please attach your patch with .diff or .patch extension ? It allows Trac to display it nicely for review. Thanks :)

Changed 10 years ago by mvexel

Attachment: cardinaldirections.patch added

Changed 10 years ago by simon04

Attachment: 9584_v2.patch added

comment:3 Changed 10 years ago by simon04

Please always try to reformat/reindent as little code as possible to ease reviewing the patch. See attachment:9584_v2.patch​ for a cleaned up version of your attachment:cardinaldirections.patch

Changed 10 years ago by simon04

Attachment: I80_west_unconnected.png added

comment:4 Changed 10 years ago by simon04

When opening relation/113177 in the editor, west roles show up to be unconnected – is this an expected behaviour?


comment:5 Changed 10 years ago by Don-vip

Is'nt there a problem with this:

                ROUNDABOUT_LEFT { 
                    @Override 
                    public Direction interpretation() { 
                        return FORWARD; 
                    } 
                }, 
                ROUNDABOUT_RIGHT { 
                    @Override 
                    public Direction interpretation() { 
                        return BACKWARD; 
                    } 
                }, 

Does it work correctly in left-driving countries as in right-driving countries ?

comment:6 in reply to:  4 ; Changed 10 years ago by mvexel

Replying to simon04:

When opening relation/113177 in the editor, west roles show up to be unconnected – is this an expected behaviour?

I get the same behavior when I change the roles from east / west to forward / backward (also in the current release client) so this is not behavior that was introduced through this patch as far as I can see. Perhaps a problem with the relation sorter?

comment:7 in reply to:  5 ; Changed 10 years ago by mvexel

Replying to Don-vip:

Is'nt there a problem with this:

...

Does it work correctly in left-driving countries as in right-driving countries ?

Not that I can see, what do you think the problem might be?

comment:8 in reply to:  6 ; Changed 10 years ago by Don-vip

Replying to mvexel:

Perhaps a problem with the relation sorter?

4 related tickets, maybe it's there: #6166, #6920, #8019, #8925

Replying to mvexel:

Not that I can see, what do you think the problem might be?

I don't know, just want to check it works correctly in both systems (haven't done any test yet with the patch)

comment:9 in reply to:  8 Changed 10 years ago by mvexel

Replying to Don-vip:

Replying to mvexel:

Perhaps a problem with the relation sorter?

4 related tickets, maybe it's there: #6166, #6920, #8019, #8925

#6166 seems relevant. #6920 to my mind describes a different situation. #8019 I think has been fixed (in the sense that reversing a way that has forward:* is correctly detected and supported, as is north:* and the other cardinal directions. #8925 I don't even understand :)

Replying to mvexel:

Not that I can see, what do you think the problem might be?

I don't know, just want to check it works correctly in both systems (haven't done any test yet with the patch)

comment:10 in reply to:  3 Changed 10 years ago by mvexel

Replying to simon04:

Please always try to reformat/reindent as little code as possible to ease reviewing the patch. See attachment:9584_v2.patch​ for a cleaned up version of your attachment:cardinaldirections.patch

Thanks and sorry about that, Eclipse messed it up and I neglected to check before I submitted.

comment:11 in reply to:  7 ; Changed 10 years ago by simon04

Replying to mvexel:

Not that I can see, what do you think the problem might be?

Why is it necessary to change the interpretation of roundabouts to something different, i.e., why not interpret ROUNDABOUT_LEFT as ROUNDABOUT_LEFT?

comment:12 Changed 10 years ago by stoecker

A first note after a look at the (cleaned) patch:
The changes in src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java are wrong. They duplicate an entry and break the changed line.

comment:13 in reply to:  12 ; Changed 10 years ago by mvexel

Replying to stoecker:

A first note after a look at the (cleaned) patch:
The changes in src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java are wrong. They duplicate an entry and break the changed line.

The only thing changed in that file is fixing two (actually technically unrelated) typos 'forwards' --> ' forward' and 'backwards' --> 'backward' - unless there is something you see that I am not?

comment:14 in reply to:  13 Changed 10 years ago by simon04

Replying to mvexel:

The only thing changed in that file is fixing two (actually technically unrelated) typos 'forwards' --> ' forward' and 'backwards' --> 'backward' - unless there is something you see that I am not?

See private static final StringSwitcher FORWARD_BACKWARD = new StringSwitcher("forward", "backward"); some lines above your change.

comment:15 in reply to:  11 ; Changed 10 years ago by stoecker

Replying to simon04:

Replying to mvexel:

Not that I can see, what do you think the problem might be?

Why is it necessary to change the interpretation of roundabouts to something different, i.e., why not interpret ROUNDABOUT_LEFT as ROUNDABOUT_LEFT?

Same question for "NONE".

comment:16 Changed 10 years ago by simon04

Since you basically consider north/west/… as forward/backward, in org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculator#determineDirection and org.openstreetmap.josm.gui.dialogs.relation.MemberTableLinkedCellRenderer#paintComponent, Direction is never NORTH/WEST/…

comment:17 in reply to:  15 Changed 10 years ago by mvexel

Replying to stoecker:

Replying to simon04:

Replying to mvexel:

Not that I can see, what do you think the problem might be?

Why is it necessary to change the interpretation of roundabouts to something different, i.e., why not interpret ROUNDABOUT_LEFT as ROUNDABOUT_LEFT?

Same question for "NONE".

Good point, those should probably go through unmapped. Let me see if I can / should submit a modified patch.

comment:18 Changed 10 years ago by simon04

To only influence the rendering of the directional keys, you might want to elaborate from

  • src/org/openstreetmap/josm/gui/dialogs/relation/sort/WayConnectionTypeCalculator.java

    diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/sort/WayConnectionTypeCalculator.java b/src/org/openstreetmap/josm/gui/dialogs/rel
    index b5cbd2e..fa48702 100644
    a b import static org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionType 
    66import static org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionType.Direction.NONE;
    77
    88import java.util.ArrayList;
     9import java.util.Arrays;
    910import java.util.List;
    1011
    1112import org.openstreetmap.josm.data.osm.Node;
    public class WayConnectionTypeCalculator { 
    5253            wct.linkPrev = i>0 && con.get(i-1) != null && con.get(i-1).isValid();
    5354            wct.direction = NONE;
    5455
    55             if (RelationSortUtils.isOneway(m)){
     56            final boolean isDirectionalForward = m.getRole().startsWith("south") || m.getRole().startsWith("west");
     57            final boolean isDirectionalBackward = m.getRole().startsWith("north") || m.getRole().startsWith("east");
     58            final boolean isDirectional = isDirectionalForward || isDirectionalBackward;
     59            if (isDirectional) {
     60                wct.direction = isDirectionalForward ? FORWARD : BACKWARD;
     61                wct.linkPrev = lastWct != null && lastWct.direction == wct.direction;
     62            }
     63
     64            if (!isDirectional && RelationSortUtils.isOneway(m)){
    5665                if (lastWct != null && lastWct.isOnewayTail) {
    5766                    wct.isOnewayHead = true;
    5867                }
    public class WayConnectionTypeCalculator { 
    6473                }
    6574            }
    6675
    67             if (wct.linkPrev) {
     76            if (!isDirectional && wct.linkPrev) {
    6877                if (lastBackwardWay != UNCONNECTED && lastForwardWay != UNCONNECTED) {
    6978                    wct = determineOnewayConnectionType(con, m, i, wct);
    7079                    if (!wct.linkPrev) {
    public class WayConnectionTypeCalculator { 
    7887                }
    7988            }
    8089
    81             if (!wct.linkPrev) {
     90            if (!isDirectional && !wct.linkPrev) {
    8291                wct.direction = determineDirectionOfFirst(i, m);
    8392                if (RelationSortUtils.isOneway(m)){
    8493                    wct.isOnewayLoopForwardPart = true;

comment:19 in reply to:  16 ; Changed 10 years ago by mvexel

Replying to simon04:

Since you basically consider north/west/… as forward/backward, in org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculator#determineDirection and org.openstreetmap.josm.gui.dialogs.relation.MemberTableLinkedCellRenderer#paintComponent, Direction is never NORTH/WEST/…

Hey Simon, don't quite understand what you're saying here, or in #18 - could you elaborate please? Happy to get in IRC as well if that works better for you. I am supplying a modified patch that should address the other concerns, thanks!

Changed 10 years ago by mvexel

Attachment: 9584_v3.patch added

new version based on comments.

comment:20 in reply to:  19 Changed 10 years ago by simon04

Summary: [Patch] Support cardinal direction member roles in LinkedCellRenderer[Patch in development] Support cardinal direction member roles in LinkedCellRenderer

Replying to mvexel:

Hey Simon, don't quite understand what you're saying here

You actually never assign Direction.NORTH to a variable, e.g., the method determineDirection in WayConnectionTypeCalculator does never return NORTH. Currently, this enum field is completely useless.

Instead, you might want to handle the north role in WayConnectionTypeCalculator as sketched in comment:18 and ignore your current patch for now.

comment:21 Changed 10 years ago by jonathanl@…

I don't understand what you're saying. The code we wrote maps from North, South, East, West to Forward/Backward with JOSM already understands.

comment:22 Changed 10 years ago by jonathanl@…

with -> which

comment:23 Changed 9 years ago by simon04

Resolution: needinfo
Status: newclosed

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.