Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extract relation #26

Draft
wants to merge 53 commits into
base: master
Choose a base branch
from
Draft

Extract relation #26

wants to merge 53 commits into from

Conversation

floscher
Copy link
Member

No description provided.

floscher
floscher commented Aug 25, 2020
View reviewed changes
Copy link
Member Author

@floscher floscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PolyglotOpenstreetmap Great that it works, I've added two minor comments, that would be nice to improve.

One thing that came to my mind is, that the current approach could struggle when one OsmPrimitive is a member of the same relation multiple times with the same role.

When the relation is like this: ABCDEFGHEJK with CDE selected, this approach would insert the extracted relation between members H and J, although the second E wasn't selected.

With editorAccess.getMemberTableModel().getSelectedIndices() you can access the selected indices and distinguish between multiple members that have the same role and the same primitive.

@PolyglotOpenstreetmap
Copy link
Collaborator

PolyglotOpenstreetmap commented Aug 25, 2020 via email

@PolyglotOpenstreetmap
I succeeded in making it work the way I want and I incorporated Flori…
ce6106d 
…an's suggestions. It sometimes still crashes on line 72. editorAccess.getEditor().reloadDataFromRelation();

Particularly if I use it on a new relation that wasn't saved yet.
@PolyglotOpenstreetmap
The relation wasn't added if member 1-... were selected, so if the fi…
e3ff059 
…rst member was part of the selection. I took away the condition. Needs more testing.
@PolyglotOpenstreetmap
Copy link
Collaborator

PolyglotOpenstreetmap commented Aug 26, 2020 via email

@PolyglotOpenstreetmap
Copy link
Collaborator

PolyglotOpenstreetmap commented Aug 26, 2020 via email

@PolyglotOpenstreetmap
refactored, it works OK most of the time, but when extracting from a …
eca82d5 
…Relation Editor that contains a sequence of ways that was extracted itself, it usually causes an exception.
@floscher floscher added this to the v2.2 milestone Sep 15, 2020
@floscher
Copy link
Member Author

@PolyglotOpenstreetmap I've created an icon in b2bf74c, so we don't need to reuse the bus icon. I'm not really satisfied, but at least it's something.

For adding to the selection pane, you could replace

pt_assistant/src/main/java/org/openstreetmap/josm/plugins/pt_assistant/PTAssistantPlugin.java

Line 198 in e61d64c

RelationEditorHooks.addActionsToMembers(group2);

with

RelationEditorHooks.addActionsToSelectio(group2); // (sic)

But I think the members pane is better, because the selection pane can also contain objects that are currently not members of the relation. I think it's better if only objects could be extracted, which are already members.

@PolyglotOpenstreetmap
Copy link
Collaborator

Hi @floscher, can you have a look at my latest attempt? I created a class for keeping track of the relations I plan to extract. For public transport routes, these extracted route relations can also have a route_ref and colour(s) tag.

For naming them I was considering to use the first and last way's names followed by the route_ref, but maybe that's more suitable as a description tag.

most likely, I will drop the street_names tag, but for now it's nice to have while debugging.

@floscher
Copy link
Member Author

Do I understand correctly that when a segment is extracted from a bus line 42, you want to reuse that extracted route relation in the superroute relation of line 1729 that goes along that same segment?

I don't think that this is how superroutes are intended to work. Also adding ref=42;1729 and colour=red;green to the common parts wouldn't be a good idea from a data standpoint. If the common part should be reused in multiple superroutes with different attributes, you should omit these tags, so they are inherited from the respective superroutes. But I'm not sure if there is such an inheritance of tags from a superroute, or if all data users take this into account or if they always just take the tags of the segments.

floscher
floscher commented Sep 18, 2020
View reviewed changes
...enstreetmap/josm/plugins/pt_assistant/actions/ExtractRelationMembersToNewRelationAction.java Outdated
RelationMember nextWayMember = null;
boolean startNewSegment = false;
PTSegmentToExtract segment = null;
for (int i = members.size()-1; i>=0; i--) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are only interested in public transport ways, I'd filter the list before looping:

final List<Way> ptWays = clonedRelation.getMembers().stream().filter(RelationMember::isWay).filter(RouteUtils::isPTWay).map(RelationMember::getWay).collect(Collectors.toList());

Then you can do this:

for (int i = members.size() - 2; i >= 1; i--) {
  final Way previousWay = ptWays.get(i - 1);
  final Way currentWay = ptWays.get(i);
  final Way nextWay = ptWays.get(i + 1);
  …
}

so you don't have to shuffle values around between these three variables.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do this, but I need to decrement the counter for each relation member encountered, as I'm using that counter to compile a list of indexes, to remove those members from the cloned relation.

I'm going to try a little bit more, but at the moment it's completely broken (again). So it will take a lot of single stepping to figure out what is going on.

Is there a way of keeping track of the indexes of those Way members when using the .filter() expression? i.e. can it be a list of Pairs?

src/main/java/org/openstreetmap/josm/plugins/pt_assistant/data/PTSegmentToExtract.java Outdated
* @param lineIdentifier The ref tag of the relation
* @param colour The colour tag of the relation
*/
public void addPTWay(RelationMember ptWay, Integer index, String lineIdentifier, String colour) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can omit the parameters lineIdentifiers and colour, these are always relation.get("ref") and relation.get("colour") at the moment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also realised that I only needed to pass the index. It's obvious from that which Way member was meant.

@PolyglotOpenstreetmap
Copy link
Collaborator

PolyglotOpenstreetmap commented Sep 18, 2020 via email

floscher
floscher commented Oct 4, 2020
View reviewed changes
src/main/java/org/openstreetmap/josm/plugins/pt_assistant/data/WaySequence.java
Comment on lines +10 to +23
public Way previousWay;
public Way currentWay;
public Way nextWay;
public Way wayAfterNextWay;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For such a tuple class it would be useful, if the fields were immutable (i.e. final), so they don't change after creation and especially can't become null unexpectedly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I make them final, my setters don't work anymore. Also I almost expect them to be null every once in a while (when currentWay is the first, last or before last highway in the route relation). currentWay should never be null however. Actually it could be null, when the stops are reached (going backwards). I changed that now. (but not committed yet)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final fields could still be null, the useful thing about them is, that during the lifetime of an object they can't change. So they can't be non-null and then later become null unexpectedly.
Yes, final fields can't have setters, all fields have to be set through the constructor.

PolyglotOpenstreetmap and others added 8 commits October 27, 2020 20:22
@PolyglotOpenstreetmap @floscher
Sometimes there is an exception. What I find odd is that this happens…
92e1bab 
… when editor.reloadDataFromRelation(); is executed, not when I'm mangling the cloned relation and not when performing the commands by passing them to the undoRedoHandler
@PolyglotOpenstreetmap @floscher
If the route relation that is divide into 2 route relations is alread…
0d4b9d9 
…y a member or superroute relations, the extracted relation most likely needs to be added to those superroute relations. So an extra option is added with the checkbox checked. It only appears when the circumstances are right for it.
@PolyglotOpenstreetmap @floscher
It worked almost the way I wanted it at some point. Trying to iron ou…
10955a9 
…t the last bugs, created a lot more complexity. I'm saving this snapshot and then I'll do lots of undo.
@floscher
Add custom icon for the extract relation action
f96c6de
@PolyglotOpenstreetmap @floscher
I got it back to a point that I'm almost happy with the result. Some …
da27d28 
…splits are not happening where I would expect them. Also, the first segment is not put into a separate relation. So it either stops too early, or it's not processing the last segment (they are processed backwards, to avoid problems with removing ways)

It is still bugged by an index out of bounds exception.

I created my very first Java Class! So quite proud of myself :-) I copied from Darya's code, so privyet, Darya!
@PolyglotOpenstreetmap @floscher
it's somewhat stable now, although there are still many things that d…
01d9ea2 
…on't work exactly the way I want. A commit before I'm going to apply Florian's suggestions
@PolyglotOpenstreetmap @floscher
I didn't like the nondescript A and B from Pair, so I created a new C…
ec66b9d 
…lass WayTriplet.

It almost works the way I want it to, now. It should also split if the first or last way in one of the parents is reached in case the line currently processed continues on a stop that is a terminus for other lines.

An odd bug is that the way that ends up in previousWay in the first iteration, stays in the original relation.

One regression is that identical relations are created multiple times, instead of being created once and then reused in the superroute
@PolyglotOpenstreetmap @floscher
Changed colours and lineIdentifiers to TreeSet. lineIdentifiers need …
d36c864 
…to be sorted using a specialised Comparator
floscher and others added 24 commits October 27, 2020 20:27
@floscher
Rename PTSegmentToExtract to RouteSegmentToExtract
0c178a8
@PolyglotOpenstreetmap @floscher
created unit test for bus line 601. It's a circular line, which reuse…
ccf71f5 
…s 3 segments
@PolyglotOpenstreetmap @floscher
I removed the code from the Action file, it's now in the RouteSegment…
e4a522d 
…ToExtract class. Some refactoring, the simpler tests in if go first and fixed a todo item.
@PolyglotOpenstreetmap @floscher
Wrote test cases for 1 complete itinerary and started on cases for an…
a1ef4f9 
…other. Almost completely rewrote the addPtWay() method
@PolyglotOpenstreetmap @floscher
renamed WayTriplet to WaySequence as it can contain 4 consecutive way…
5372fa0 
…s now
@PolyglotOpenstreetmap @floscher
All the unit tests are working up to this point. Rewrote isItineraryI…
742265a 
…nSameDirection so it can both work with previous and next ways, as with start and end node of currentWay for when one of the ways is null;
@PolyglotOpenstreetmap @floscher
Added some more unit tests and needed to check that if one of the par…
deb7c6d 
…ent ways is the first way, that it also was for an itinerary going in the same direction.
@PolyglotOpenstreetmap @floscher
Added a lookup for itineraryWays per relation, so it only needs to be…
28f11b6 
… calculated once. They are needed over and over again for each of the parent route relations of all the member ways.

Modified WaySequence according to Florian's suggestions.

Stopped excluding the relation that is processed from the parent route relations.
@PolyglotOpenstreetmap @floscher
In some cases the membershipcount is needed of all the (parent) route…
4bd2b71 
… relations, in others, it's only the routes that go in the same sense that count.

All tests for 2 lines that make a circle around the city, but which at the railway station and the bus station near the hospital reuse ways in the same or the opposite sense, pass now.

Now I'm going to do some interactive testing on other bus lines.

I also found a bus line that wasn't mapped correctly, so the test osm file is also updated
@PolyglotOpenstreetmap @floscher
Added another line, which has variants with an extra spoon, which nee…
4bcfa3f 
…ds special treatment. When I had fixed that, it gave problems at roundabouts and where the bus forked on oneway stretches, but that is solved as well now.

Cleaned up the test code, so IntelliJ has a minimal amount of warnings on it.

Also found some errors in the OSM file.
@PolyglotOpenstreetmap @floscher
Added connectivity checks to WaySequence and wrote JavaDoc for its me…
af2ab8c 
…thods. If there is a gap, hasGap gets set.

I started to check this in findPreviousAndNextWayInRoute and came to the conclusion that I had to change the OSM file and a few test results.

I also renamed 2 variables to improve readability.
@PolyglotOpenstreetmap @floscher
I have no idea how to resolve this exception. It always complains abo…
04acfb0 
…ut Index xxx out of bounds for length yyy. Where xxx is the original length, and yyy the new size of the members list.

java.lang.IndexOutOfBoundsException: Index 157 out of bounds for length 70
	at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
	at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
	at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:248)
	at java.base/java.util.Objects.checkIndex(Objects.java:373)
	at java.base/java.util.ArrayList.get(ArrayList.java:426)
	at java.base/java.util.stream.IntPipeline$1$1.accept(IntPipeline.java:180)
	at java.base/java.util.Spliterators$IntArraySpliterator.forEachRemaining(Spliterators.java:1032)
	at java.base/java.util.Spliterator$OfInt.forEachRemaining(Spliterator.java:699)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
	at org.openstreetmap.josm.gui.dialogs.relation.MemberTableModel.getSelectedMembers(MemberTableModel.java:516)
	at org.openstreetmap.josm.gui.dialogs.relation.MemberTableModel.hasIncompleteSelectedMembers(MemberTableModel.java:416)
	at org.openstreetmap.josm.gui.dialogs.relation.actions.DownloadSelectedIncompleteMembersAction.updateEnabledState(DownloadSelectedIncompleteMembersAction.java:46)
	at org.openstreetmap.josm.gui.dialogs.relation.actions.AbstractRelationEditorAction.tableChanged(AbstractRelationEditorAction.java:76)
	at java.desktop/javax.swing.table.AbstractTableModel.fireTableChanged(AbstractTableModel.java:297)
	at java.desktop/javax.swing.table.AbstractTableModel.fireTableDataChanged(AbstractTableModel.java:199)
	at org.openstreetmap.josm.gui.dialogs.relation.MemberTableModel.populate(MemberTableModel.java:227)
	at org.openstreetmap.josm.gui.dialogs.relation.GenericRelationEditor.populateModels(GenericRelationEditor.java:327)
	at org.openstreetmap.josm.gui.dialogs.relation.GenericRelationEditor.reloadDataFromRelation(GenericRelationEditor.java:320)
	at org.openstreetmap.josm.plugins.pt_assistant.actions.ExtractRelationMembersToNewRelationAction.actionPerformed(ExtractRelationMembersToNewRelationAction.java:159)
	at java.desktop/javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:1967)
	at java.desktop/javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2308)
	at java.desktop/javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:405)
	at java.desktop/javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:262)
	at java.desktop/javax.swing.plaf.basic.BasicButtonListener.mouseReleased(BasicButtonListener.java:279)
	at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:297)
@PolyglotOpenstreetmap @floscher
as route relations get converted to superroute relations, sub route r…
d33a855 
…elations start to appear. It's important to also consider the parents of these sub route relations when compiling a list of parent route relations for the same direction of travel.
@PolyglotOpenstreetmap @floscher
Added tests for cycle route.
5448f8f 
Removed constructor that takes DataSet as a parameter and created a setter for it. It seems cleaner.
@PolyglotOpenstreetmap @floscher
Rewrote the entire test suite. It works with an array of known values…
41a9649 
… now.

I also created code to autogenerate such tests for a given relation.

Oddly some of these tests are generated wrongly where bus 3 goes through the station, but not when processing line 3 itself. I haven't been able to figure out where this comes from.
@PolyglotOpenstreetmap @floscher
During live testing I get an Index out of bounds exception, when proc…
11a99a3 
…essing line 433 (and 333). I also regularly get java.lang.IllegalArgumentException: Width and height must be >= 0 while browsing the map, with a relation editor window open.

Annoyingly I can't figure out how to reproduce these exceptions in my unit tests.
@PolyglotOpenstreetmap @floscher
Moved isItineraryInSameDirection to WaySequence class.
6b01f01 
Intermediary commit before simplifying isItineraryInSameDirection
@PolyglotOpenstreetmap @floscher
Some refactoring
977d54d 
Moved isItineraryInSameDirection to WaySequence class and renamed it to compareTraversal.

created a separate method to keep traversalSense property up-to-date.
@PolyglotOpenstreetmap @floscher
resolved some warnings from IntelliJ
dbdd778
@floscher
Simplify dataset loading in RouteSegmentToExtractTest
4c3ce56
@PolyglotOpenstreetmap @floscher
Tried to sanitize the RouteSegmentToExtractTest class. I didn't test …
f66483b 
…creating a new set of knownValues.

What is odd, is that now all the tests are passing, whereas before I did this some were not passing. So either it's not testing as exhaustively, or there is an error in the known values. But those I didn't touch.
@PolyglotOpenstreetmap @floscher
Some changes made while doing pair programming with Florian.
2b52d4d
@PolyglotOpenstreetmap @floscher
Running the loop down to 0 seems to work better when splitting in seg…
12158e8 
…ments
@PolyglotOpenstreetmap @floscher
Changed the order of creation of clonedRelation. The way it was, it w…
5e6e728 
…as not doing/testing anything.
floscher and others added 5 commits October 28, 2020 10:03
@floscher
Merge master branch into extract_relation
d8e83b5
@PolyglotOpenstreetmap
reduced file size of test file to 5.5 MB by purging ways. The relatio…
4305ff8 
…ns need to stay, as they matter for splitting and for determining which lines to mention in the note tag.

Fixed a bug in WaySequence

The tests were rewritten from scratch. They all pass now, but interactive testing is needed to know whether they test the correct behaviour. Atm they test the curent behaviour of the code, as they were generated using it.
@PolyglotOpenstreetmap
Reworked the tests. For removing members from a relation, it makes mo…
616e955 
…st sense to work backwards. But having the tests from last member to first is confusing. So now it's incrementing and moving forward through the members. This changes much in the logic about when to split, but it's a lot easier to follow.

To make this possible, a list of indices to remove and a list of relations to add are and this is done in outside of the main loop.
It seems to also solve the index out of bounds issue. It does not solve the problem where it complains when browsing through the elements of a relation that had its members reloaded:
ava.lang.IllegalArgumentException: Width and height must be >= 0
@PolyglotOpenstreetmap
In ExtractRelationMembersToNewRelationAction I made an attempt to get…
ac95d6d 
… rid of the Index out of Bounds exception. The attempt failed.

Fixed some bugs in RouteSegmentToExtract that came from reversing the order in which members are evaluated

Tried to add functionality to the test code generator to copy to the clipboard. Unfortunately it doesn't work on Linux or on OpenJDK.

I had to change the OSM file. Some data that is important for the decision where to split was missing from it.
@PolyglotOpenstreetmap
It seems like I succeeded in suppressing the IndexOutOfBounds excepti…
0355688 
…on, so the user is not affected by it. It's still there though, but I can't figure out what the root cause of it is.

I see this one a lot less:
java.lang.IllegalArgumentException: Width and height must be >= 0

But it still happens occasionally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PolyglotOpenstreetmap PolyglotOpenstreetmap PolyglotOpenstreetmap left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.2
Development

Successfully merging this pull request may close these issues.
2 participants
@floscher @PolyglotOpenstreetmap