[WIP PATCH] Add support for Mapbox Vector Tile

[Don-vip]

OpenInfraMap dropped its classic tile servers to switch to vector tiles.

If we want to display it in JOSM we must first support ​MVT format, built on PBF.

Old Maps/Worldwide entries for reference and possible future restoration:

• OpenInfraMap Telecoms {view}
• OpenInfraMap Power {view}
• OpenInfraMap Petroleum {view}

    <entry overlay="true">
        <name>OpenInfraMap Telecoms</name>
        <id>openinframap-telecoms</id>
        <category>osmbasedmap</category>
        <type>tms</type>
        <description lang="en">Overlay imagery for telecoms infrastructure mapping in OSM. Updated daily.</description>
        <url>https://tiles-{switch:a,b,c}.openinframap.org/telecoms/{zoom}/{x}/{y}.png</url>
        <attribution-text>© OpenInfraMap.org</attribution-text>
        <attribution-url>https://www.openinframap.org/</attribution-url>
        <max-zoom>19</max-zoom>
        <valid-georeference>true</valid-georeference>
    </entry>

    <entry overlay="true">
        <name>OpenInfraMap Power</name>
        <id>openinframap-power</id>
        <category>osmbasedmap</category>
        <type>tms</type>
        <description lang="en">Overlay imagery for power infrastructure mapping in OSM. Updated daily.</description>
        <url>https://tiles-{switch:a,b,c}.openinframap.org/power/{zoom}/{x}/{y}.png</url>
        <attribution-text>© OpenInfraMap.org</attribution-text>
        <attribution-url>https://www.openinframap.org/</attribution-url>
        <max-zoom>19</max-zoom>
        <valid-georeference>true</valid-georeference>
    </entry>

    <entry overlay="true">
        <name>OpenInfraMap Petroleum</name>
        <id>openinframap-petroleum</id>
        <category>osmbasedmap</category>
        <type>tms</type>
        <description lang="en">Overlay imagery for petroleum infrastructure mapping in OSM. Updated daily.</description>
        <url>https://tiles-{switch:a,b,c}.openinframap.org/petroleum/{zoom}/{x}/{y}.png</url>
        <attribution-text>© OpenInfraMap.org</attribution-text>
        <attribution-url>https://www.openinframap.org/</attribution-url>
        <max-zoom>19</max-zoom>
        <valid-georeference>true</valid-georeference>
    </entry>

[GerdP]

I don't see how this is related to the pbf plugin. IIGTR their format just also uses Google protobuf to specify the format.

[pyrog]

Will be very useful to display vector tiles in JOSM :)
And to query them too.

​iD do this :)
But unfortunately this editor is not powerful as JOSM.

[taylor.smock]

I'm starting to look at the requirements to implement this.

Questions:

• Do we want to add com.google.protobuf (~1.6 MB, so a bit more than 10% of the current JOSM size) or attempt to write our own protobuf reader? (See ​Google encoding docs for encoding documentation, I've started on a basic implementation, but it may be better/easier to use the com.google.protobuf library)
• If we use com.google.protobuf, do we want to merge the pbf plugin into JOSM core? (I doubt this -- there is another dependency for it)

[stoecker]

Replying to taylor.smock:

I'm starting to look at the requirements to implement this.

Questions:

• Do we want to add com.google.protobuf (~1.6 MB, so a bit more than 10% of the current JOSM size) or attempt to write our own protobuf reader? (See ​Google encoding docs for encoding documentation, I've started on a basic implementation, but it may be better/easier to use the com.google.protobuf library)
• If we use com.google.protobuf, do we want to merge the pbf plugin into JOSM core? (I doubt this -- there is another dependency for it)

I'd rather say the support should go into a plugin as well when it needs such heavy libraries.

[taylor.smock]

Replying to stoecker:

[...]

I'd rather say the support should go into a plugin as well when it needs such heavy libraries.

I've started working on an custom implementation for protobuf. Based off of what I've done so far, I have no idea why the official protobuf library is 1.6 MB. Probably dependencies, TBH. (Maybe Guava?).

Anyway, WIP Patch notes:

• Packed data is not yet implemented (so no tags/geometry)
• I haven't started hooking anything up to imagery layers
• I'm using ​https://tiles3.mapillary.com/v0.1/14/3251/6258.mvt as test data (also ​https://tiles3.mapillary.com/v0.1/14/3251/6257.mvt and a ​local county parcel tile). The latter two are not yet used in tests. I'll attach the files used once I take this out of WIP status.

[taylor.smock]

WIP Patch. DO NOT APPLY. Parsing of Mapbox Vector Tiles partially works. Packed fields do not yet work (specifically, tags and geometry)

[stoecker]

I've started working on an custom implementation for protobuf. Based off of what I've done so far, I have no idea why the official protobuf library is 1.6 MB. Probably dependencies, TBH. (Maybe Guava?).

Very common effect. Use a library for a 5 line function and another one and another one and ...

If support for protobuf can be that small maybe pbf plugin can go into core as well ;-)

[taylor.smock]

Initial rendering work (currently only works on zoom level 14)

[taylor.smock]

Now I just have to finish (basic) rendering.

I'm having lots of fun trying to figure out why zoom level 14 works, but no other zoom does (I'm figuring I probably have to figure out some AffineTransform for it).

[taylor.smock]

Rendering works on all zoom levels (quirks are present at low zoom levels)

[taylor.smock]

What still needs to be done:

• Check rendering on HiDPI monitor (surprisingly, the magic number works on Fedora with Java version: 15.0.2+7, Red Hat, Inc., OpenJDK 64-Bit Server VM and Screen: :0.0 3840×2160 (scaling 1.00×1.00) :0.1 3840×2160 (scaling 1.00×1.00))
• Extend tests, and check random areas to ensure everything works well
• Add some kind of method for layers to have arbitrary rendering (this should probably be a new ticket, we should probably support ​https://docs.mapbox.com/mapbox-gl-js/style-spec )
• Ability to query data on layer
• Ability to toggle vector tile layers (not JOSM layers)
• Cleanup TODOs in patch
• Add support for MVT layers in imagery index parser (may already be done)
• Handle overlap of areas better (see OpenInfraMap)
  • ​https://osm.org/way/230300564
  • ​https://osm.org/way/666293900

Test layers:

• mvt[14]:​https://tiles3.mapillary.com/v0.1/{zoom}/{x}/{y}.mvt
• mvt[17]:​https://openinframap.org/tiles/{zoom}/{x}/{y}.pbf

[taylor.smock]

Fix rendering issue with regards to a multipolygons being incorrectly rendered. This is probably good enough for initial testing (if this patch gets merged, functionality should be hidden behind expert mode).

[Don-vip]

Nice! I'll take a look for next version.

[taylor.smock]

Thanks. I don't know if you have a HiDPI screen or not, but I've only tested on the following combinations:

• Fedora 33 with 4K monitors (Java 15)
• MacOS 10.15 with 1080p and 1800p monitors (Java 11)

I'm worried that the "magic number" I have (32768 or 2^15) only works for specific configurations.

Notes:

• Quirks are present at low zoom levels (the tiles load and then disappear -- I need to debug this)
• Only static styling is implemented (so points are green non-filled circles, lines are red, and areas are yellow).
• Only the base URL is supported -- stylesheets that conform to ​https://docs.mapbox.com/mapbox-gl-js/style-spec are not yet supported, although they contain both template URLs for the tiles and style information
• I'm currently debating over whether or not I want to convert tiles to a DataSet and reuse the rendering pipeline for mapcss (this is tempting, I'd probably have to add a few methods for getWays(bbox, zoom) and have some zoom-specific buckets or something, just to reduce cycles)
• There is no deduplication of features. So if a line or polygon crosses a tile boundary (see ​https://www.openstreetmap.org/way/666293900 with ​https://www.openstreetmap.org/way/666293900 for an example).
• There could probably be some performance enhancements

[taylor.smock]

Switch to using an OsmData class (VectorDataSet) along with Vector{Node,Way,Relation} in order to make styling easier (there is a significant performance hit with this patch).

[taylor.smock]

Checkpoint -- style generation should be good enough, although it is not yet currently used

[taylor.smock]

Styling is used now, icons need some additional special handling (not yet implemented)

[simon04]

Related Java library: ​https://github.com/ElectronicChartCentre/java-vector-tile

[taylor.smock]

Replying to simon04:

Related Java library: ​https://github.com/ElectronicChartCentre/java-vector-tile

Thanks for posting this. I'll check the license to see if I can reuse parts of the test suite, but the overall dependency list for it makes it a no-go for JOSM (unless comment:4 / comment:5 no longer apply -- one of the dependencies is google protobuf 3.9.1 (1.6mb)).

[simon04]

Err, I haven't checked its dependencies. The library size is still relevant, see discussion starting at ticket:19972#comment:3.

[taylor.smock]

Don't worry about it. The library size is the whole reason why I've re-implemented the protobuf reader. Just about everyone who has implemented some kind of vector tile library has used Google Protobuf, which means I couldn't reuse a library.

[taylor.smock]

PBFs used for testing

[taylor.smock]

In attachment:17177.7.protobuf.patch​, the following files in the attachment:pbf.tar.xz​ are used:

• pbf/mapillary/14/3251/6258.mvt (ProtoBufTest#testRead_14_3251_6258)
• pbf/openinframap/17/26028/50060.pbf (ProtoBufTest#testRead_17_26028_50060)

Additional notes:
There are three TODOs remaining in attachment:17177.7.protobuf.patch​

Two are the same (ProtoBufRecord#asFixed{32,64}), and has to do with what should happen if the actual wiretype isn't the appropriate type. I'm inclined to throw an exception, but feedback would be appreciated on that.

The last is for ProtoBufParser#convertLong, and has to do with booleans. The specification for protobuf buffers indicates that booleans (and enums) are of the VarInt type, and I'm inclined to treat booleans like I'm treating enums, and let the consumer convert the number to a boolean.

Beyond those TODO's, I'm not intending on making further changes to the protobuf code.

EDIT: I'm splitting out the PBF code now, since it is almost 1700 lines, and the total patch size is currently ~6600 lines, so I think I should start splitting it up into self-contained changes, where possible.

[taylor.smock]

Protobuf only classes and tests (tests cover most everything, including most mutations). Files needed for tests will be uploaded separately.

[taylor.smock]

Tests for org.openstreetmap.josm.data.imagery.vectortile.mapbox and org.openstreetmap.josm.data.protobuf (tests for styling is expected to fail as more support is added for styling)

[taylor.smock]

attachment:17177.8.patch​ should not require any additional files from attachment:pbf.tar.xz​ (see comment:17).

Notes:

• Removed API key from mapillary.json (I'm using _apiKey_ there now)
• I have not checked that adding a new MVT entry in the imagery "just works"
• There are some perf issues, but this may be due to the image rendering (there was also a hit when I converted it to OSM data, and used the generated style)

[taylor.smock]

It looks like someone will need to update the Wiki validation page for this:
Warning: Invalid Wiki page: XML validation for map failed: Element '{http://josm.openstreetmap.de/maps-1.0}type': [facet 'enumeration'] The value 'mvt' is not an element of the set {'wms', 'wms_endpoint', 'wmts', 'tms', 'bing', 'scanex'}. (line 0)

It looks like it should just work though.

@don-vip: ​https://gitlab.com/smocktaylor/josm/-/merge_requests/2 -- I figure it might be a little easier for you to keep track of what you have looked at there. I'll continue posting patches here, so you can look at whatever works best for you.

I really don't want to keep touching it (I think 8k lines is a bit much for most people to review), and I haven't seen any major issues in testing. The only major issue I'm seeing is due to dataset locking (to avoid concurrent modification exceptions).

[taylor.smock]

If you want to play around with it, I'm using ​https://gitlab.com/smocktaylor/josm/-/snippets/2106027/raw/master/mapillary.json as a test URL.

[simon04]

PBF support in JOSM core is very welcome. :))

I cannot possibly review a patch of 8k lines of code in depth, but I don't object its inclusion unless it causes instability to other components (which I doubt).

After playing around with ​https://gitlab.com/smocktaylor/josm/-/commit/d5519ae02bc1462f6fcb03ce679c298eb3983a4d for 10 minutes and wildly zooming around, panning, removing layers I managed to only (!) produce a single exception. So I'm fairly optimistic and pro inclusion.

Revision:17788
Is-Local-Build:true
Build-Date:2021-04-16 00:10:13

Identification: JOSM/1.5 (17788 SVN en) Mac OS X 11.2.3
OS Build number: macOS 11.2.3 (20D91)
Memory Usage: 1741 MB / 2048 MB (566 MB allocated, but free)
Java version: 16+36, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.apple.laf.AquaLookAndFeel
Screen: Display 1 1440×900 (scaling 2,00×2,00) Display 2 3008×1692 (scaling 2,00×2,00)
Maximum Screen Size: 3008×1692
Best cursor sizes: 16×16→16×16, 32×32→32×32
Environment variable LANG: en_IE.UTF-8
System property file.encoding: UTF-8
System property sun.jnu.encoding: UTF-8
VM arguments: [-Djosm.home=<josm.pref>/]
Program arguments: [--set=expert=true, --set=iso.dates=true]

Last errors/warnings:
- 00086,095 E: Handled by bug report queue: org.openstreetmap.josm.tools.JosmRuntimeException: failed to remove primitive: org.openstreetmap.josm.data.vector.VectorNode@31debbdb



=== REPORTED CRASH DATA ===
BugReportExceptionHandler#handleException:
No data collected.

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: MVT-downloader-8 (79)
org.openstreetmap.josm.tools.JosmRuntimeException: failed to remove primitive: org.openstreetmap.josm.data.vector.VectorNode@31debbdb
	at org.openstreetmap.josm.data.osm.QuadBucketPrimitiveStore.removePrimitive(QuadBucketPrimitiveStore.java:128)
	at org.openstreetmap.josm.data.vector.DataStore$LocalQuadBucketPrimitiveStore.removePrimitive(DataStore.java:42)
	at org.openstreetmap.josm.data.vector.DataStore.removePrimitive(DataStore.java:102)
	at org.openstreetmap.josm.data.vector.VectorDataStore.pointToNode(VectorDataStore.java:203)
	at org.openstreetmap.josm.data.vector.VectorDataStore.pathIteratorToObjects(VectorDataStore.java:253)
	at org.openstreetmap.josm.data.vector.VectorDataStore.pathToWay(VectorDataStore.java:218)
	at org.openstreetmap.josm.data.vector.VectorDataStore.lambda$addTile$7(VectorDataStore.java:310)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.Collections$UnmodifiableCollection.forEach(Collections.java:1087)
	at org.openstreetmap.josm.data.vector.VectorDataStore.lambda$addTile$9(VectorDataStore.java:304)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.Collections$UnmodifiableCollection.forEach(Collections.java:1087)
	at org.openstreetmap.josm.data.vector.VectorDataStore.addTile(VectorDataStore.java:299)
	at org.openstreetmap.josm.data.vector.VectorDataSet.lambda$addTileData$22(VectorDataSet.java:477)
	at org.openstreetmap.josm.data.vector.VectorDataSet.tryWrite(VectorDataSet.java:508)
	at org.openstreetmap.josm.data.vector.VectorDataSet.addTileData(VectorDataSet.java:477)
	at org.openstreetmap.josm.gui.layer.imagery.MVTLayer.finishedLoading(MVTLayer.java:276)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.MVTTile.lambda$loadImage$1(MVTTile.java:66)
	at org.openstreetmap.josm.tools.ListenerList.fireEvent(ListenerList.java:162)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.MVTTile.loadImage(MVTTile.java:66)
	at org.openstreetmap.josm.data.imagery.TMSCachedTileLoaderJob.tryLoadTileImage(TMSCachedTileLoaderJob.java:327)
	at org.openstreetmap.josm.data.imagery.TMSCachedTileLoaderJob.loadingFinished(TMSCachedTileLoaderJob.java:209)
	at org.openstreetmap.josm.data.cache.JCSCachedTileLoaderJob.finishLoading(JCSCachedTileLoaderJob.java:266)
	at org.openstreetmap.josm.data.cache.JCSCachedTileLoaderJob.run(JCSCachedTileLoaderJob.java:235)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
	at java.base/java.lang.Thread.run(Thread.java:831)

@team, any concerns for including PBF support in JOSM?

[taylor.smock]

Replying to simon04:

PBF support in JOSM core is very welcome. :))

I cannot possibly review a patch of 8k lines of code in depth, but I don't object its inclusion unless it causes instability to other components (which I doubt).

Considering I tried to avoid touching other components, it shouldn't. But there were some unavoidable changes. And I'm sorry about the 8k lines (I don't expect anyone to review it in depth, and I probably should have switched to git for the development earlier, just so that I wouldn't forget why I did some things).

Probably the biggest file to look at is ​src/org/openstreetmap/josm/data/cache/JCSCachedTileLoaderJob.java, followed by ​src/org/openstreetmap/josm/data/imagery/TMSCachedTileLoaderJob.java.

Also, WaySegment implements IWaySegment, and RelationSorter has been generified (I want to merge lines that cross tile boundaries for future selection/querying, the merge line code is a bit iffy right now -- not the code in RelationSorter).

I'm also synchronizing on the primitive in the paint loop (I'm using a method to get a value to synchronize on, which defaults to this, I don't remember why I did this anymore, but I do remember it fixed a bug I was running into).

As far as tests go, org.openstreetmap.josm.data.imagery.vectortile and org.openstreetmap.josm.data.protobuf should be pretty much close to 95% tested (note: these are largely mutation tested with PIT).

The least well-tested part of the new code is the org.openstreetmap.josm.data.vector classes. Which brings me to:

After playing around with ​https://gitlab.com/smocktaylor/josm/-/commit/d5519ae02bc1462f6fcb03ce679c298eb3983a4d for 10 minutes and wildly zooming around, panning, removing layers I managed to only (!) produce a single exception. So I'm fairly optimistic and pro inclusion.

[snip]

Thanks for the stack trace. I'll see if I can repro so I can write a test case. Probably a race condition.

@team, any concerns for including PBF support in JOSM?

I'm not @team, but I should note that this does not support the OSM PBF format (although I or someone else could write a parser), just the Mapbox Vector Tile PBF support.

The patch also adds support for some parts of the Mapbox Vector Style (I translate it into mapcss). Of note, since we have spent some time moving from png to svg in JOSM, I have to download a png file, and then convert it into sprites for use with mapcss. Rather unfortunately, the style spec specifices png for the format.

[stoecker]

Replying to taylor.smock:

It looks like someone will need to update the Wiki validation page for this:
Warning: Invalid Wiki page: XML validation for map failed: Element '{http://josm.openstreetmap.de/maps-1.0}type': [facet 'enumeration'] The value 'mvt' is not an element of the set {'wms', 'wms_endpoint', 'wmts', 'tms', 'bing', 'scanex'}. (line 0)

The wiki validation is based in the XSD contained in JOSM repo so as soon as the XSD is updated with new stuff validation also will be.

Which on the other hand means using mvt will cause older JOSM instances to fail when validating the downloaded maps file :-(

[stoecker]

P.S. I have no objections also to include Work in progress of such scope. So there is really no need to wait for a "complete" feature!

[taylor.smock]

Add MVT to maps.xsd

[taylor.smock]

Replying to stoecker:

Which on the other hand means using mvt will cause older JOSM instances to fail when validating the downloaded maps file :-(

Good to know. It didn't look like it would cause older JOSM instances to fail (case "type" in ImageryReader sets skipEntry to true if the ImageryType cannot be found). Unless you are talking about versions that are older than 2 years old? (I think that even versions 10 years old would be OK, but the last change to the lines of interest was in 2018)

That being said, if JOSM validates the maps xml against the XSD built into the jar, then we should probably prohibit the addition of MVT endpoints to the background list for a few months.

[stoecker]

Replying to taylor.smock:

That being said, if JOSM validates the maps xml against the XSD built into the jar, then we should probably prohibit the addition of MVT endpoints to the background list for a few months.

I believe it does.

But there is an easier solution: Simply deliver different content for older JOSMs 🙄

[taylor.smock]

Replying to simon04:

After playing around with ​https://gitlab.com/smocktaylor/josm/-/commit/d5519ae02bc1462f6fcb03ce679c298eb3983a4d for 10 minutes and wildly zooming around, panning, removing layers I managed to only (!) produce a single exception. So I'm fairly optimistic and pro inclusion.
[...]

I've fixed this issue. This was caused by attempting to merge ways (largely for when a line is split between tiles).

I really wanted to merge ways in different tiles (mostly for easier querying, whenever I get around to adding that), but the vector tile specification states "A feature MAY contain an id field. If a feature has an id field, the value of the id SHOULD be unique among the features of the parent layer." Note the SHOULD be unique. So I cannot do the merges based off of id, which means I had to remove the deduplication code. Which also sped up the download and render process.

[simon04]

I'll review/apply this patch for milestone:21.05. Thank you for your patience :)

[taylor.smock]

Replying to simon04:

I'll review/apply this patch for milestone:21.05. Thank you for your patience :)

No problem. I assume >14 days since last response to mean its fallen out of mind.

Anyway, I just made a few changes this morning (if you are interested).

I added two new generic interfaces (essentially clones of existing classes, see ​df97b923cfdb8a63fe8ac3f7d835b177230982af) so that VectorDataSets could have listeners for selection changes.

I haven't been able to figure out how to reuse the generic interface in DataSet without breaking compatibility, unfortunately, so we have duplicated code.

[simon04]

I've left a review at ​https://gitlab.com/smocktaylor/josm/-/merge_requests/2 and found a few minor issues/suggestions. Please let me know when the patch set is ready to be committed. Then, should I commit the (currently) 27 commits separately after modifying the commit message to "see #17177 - ... (patch by taylor.smock), or squashing them to a single commit?

[taylor.smock]

Thanks for the review simon04. I've applied some of the suggested patches (thanks for those).

I will let you know when the patch set is ready to be committed (I actually thought it was ready Monday, but then I started working on using it in the Mapillary plugin, and I wasn't happy with the performance I was seeing, so a good chunk of the work I did for combining objects across tiles has been scrapped -- it had too much locking to avoid ConcurrentModificationExceptions).

As far as the numerous commits go, I don't really care. That being said, it might be a good idea to squash them together to make bisecting easier.

I have a tendency to commit early, and commit often (usually the project compiles at that point, which is not the case here as the first three commits or so were effectively split out from the original non-version controlled patch). See the ​bus factor for why I try to commit early and often.

[taylor.smock]

@simon04: I'm done. Anything else I do with it should be done in separate tickets.

Please note that I've changed the Filter classes so that they are more generic. See ​b6c5d2943166c491a29acff5ffeb30df2f362e66.

Possibly affected plugins:

• configuration
• Mapillary
• MapWithAI

It doesn't look like any rebuild is necessary for them (it "just" worked with an older version of the Mapillary plugin, anyway).

[simon04]

In 17862/josm:

fix #17177 - Add support for Mapbox Vector Tile (patch by taylor.smock)

Signed-off-by: Taylor Smock <tsmock@…>

[simon04]

I've applied attachment:17177-r17862.patch​, squashed it​ into a single commit and committed as r17862.

[taylor.smock]

Replying to simon04:

I've applied attachment:17177-r17862.patch​, squashed it​ into a single commit and committed as r17862.

Thank you. I'll try to update the Maps documentation in a few days, with examples (hopefully).

[simon04]

In 17867/josm:

see #17177 - fix @since xxx

[simon04]

In 17879/josm:

see #17177 - Fix LayersTest.testSymbol

[holgermappt]

During translation of the strings I came across the AddMVTLayerPanel constructor (file src/org/openstreetmap/josm/gui/preferences/imagery/AddMVTLayerPanel.java, line 31):

        add(new JLabel(tr("{0} Make sure OSM has the permission to use this service", "1.")), GBC.eol());
        add(new JLabel(tr("{0} Enter URL (may be a style sheet url)", "2.")), GBC.eol());

What is the idea behind the tr("{0} Some text", "N.") syntax? Do you assume that numbered lists start with "1." in all languages? That assumption is wrong. Please re-write this as tr("N. Some text"). In general it is not a good idea to build a sentence from pieces, for translation it is better to write the full text in one string.
Please try to be consistent. If the first URL is written in capital letters (which is right for abbreviations), then the second url should be written the same way.

[taylor.smock]

The first tr string is literally the same string as https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/gui/preferences/imagery/AddWMTSLayerPanel.java#L42, and I reused the same format for the additional translations.

EDIT: As far as the second point goes, you may have a point. Like the first, it is copy-pasted from AddWMTSLayerPanel, but with the addition of may be a style sheet url.

[holgermappt]

Replying to taylor.smock:

The first tr string is literally the same string as https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/gui/preferences/imagery/AddWMTSLayerPanel.java#L42, and I reused the same format for the additional translations.

OK, then ignore my comment. For a translator it is confusing to translate a string with a placeholder in front of it that seems unrelated. But I understand that there is a wish to re-use the strings to keep translation effort to a minimum.

[simon04]

Relevant commit regarding i18 strings: r13591

Regressions of this patch: java.lang.NoSuchFieldError: org.openstreetmap.josm.data.osm.WaySegment#way

• #20876
• #20882
• #20885

Potentially affected plugins:

alignways/src/org/openstreetmap/josm/plugins/alignways/AlignWaysAlgnSegment.java
alignways/src/org/openstreetmap/josm/plugins/alignways/AlignWaysCmdKeepAngles.java
alignways/src/org/openstreetmap/josm/plugins/alignways/AlignWaysCmdKeepLength.java
alignways/src/org/openstreetmap/josm/plugins/alignways/AlignWaysSegment.java
auto-tools/src/org/openstreetmap/josm/plugins/auto_tools/actions/SplittingTool.java
building_generalization/src/org/openstreetmap/josm/plugins/buildinggeneralization/BuildingGeneralizationAction.java
building_generalization/src/org/openstreetmap/josm/plugins/buildinggeneralization/ShapeMath.java
buildings_tools/src/org/openstreetmap/josm/plugins/buildings_tools/DrawBuildingAction.java
cadastre-fr/src/org/openstreetmap/josm/plugins/fr/cadastre/actions/mapmode/Address.java
CustomizePublicTransportStop.git/src/org/openstreetmap/josm/plugins/customizepublictransportstop/CreateNewStopPointOperation.java
CustomizePublicTransportStop/src/org/openstreetmap/josm/plugins/customizepublictransportstop/CreateNewStopPointOperation.java
improve-way/src/org/openstreetmap/josm/plugins/improveway/ImproveWayAccuracyAction.java
improve-way/src/org/openstreetmap/josm/plugins/improveway/ImproveWayAccuracyHelper.java
Mapillary/src/main/java/org/openstreetmap/josm/plugins/mapillary/data/mapillary/AdditionalInstructions.java
pt_assistant/src/main/java/org/openstreetmap/josm/plugins/customizepublictransportstop/CreateNewStopPointOperation.java
pt_assistant/src/main/java/org/openstreetmap/josm/plugins/pt_assistant/actions/AddStopPositionAction.java
pt_assistant/src/main/java/org/openstreetmap/josm/plugins/pt_assistant/actions/DoubleSplitAction.java
shapetools/src/org/openstreetmap/josm/plugins/shapetools/DrawableSegmentBuilding.java
shapetools/src/org/openstreetmap/josm/plugins/shapetools/DrawableSegment.java
shapetools/src/org/openstreetmap/josm/plugins/shapetools/DrawableSegmentRoad.java
shapetools/src/org/openstreetmap/josm/plugins/shapetools/ShapeEvents.java
shapetools/src/org/openstreetmap/josm/plugins/shapetools/ShapeMath.java
trustosm/src/org/openstreetmap/josm/plugins/trustosm/gui/dialogs/TrustDialog.java

[taylor.smock]

Replying to simon04:

Regressions of this patch: java.lang.NoSuchFieldError: org.openstreetmap.josm.data.osm.WaySegment#way

• #20876
• #20882
• #20885

Would it be worth doing something like this in WaySegment?

public final Way way;
public WaySegment(Way way, int i) {
super(way, i);
this.way = super.way;
}

I'd have to check and see if that compiles, and there are issues with that (it masks the IWaySegment way).

EDIT: And it compiles. The patch I attached marks the fields as deprecated, and points at IWaySegment. It does (significantly) increase the number of compile time warnings though.

[taylor.smock]

Binary compatibility

[taylor.smock]

Affected plugins (will be updated as I go through them):

• alignways
• auto-tools -- ​https://github.com/JOSM/auto-tools/pull/26
• building_generalization -- ​https://github.com/JOSM/Building-Generalization/pull/3
• buildings_tools -- see #20885
• cadastre-fr
• CustomizePublicTransportStop
• improve-way -- ​https://github.com/JOSM/improve-way/pull/3
• Mapillary -- I'm going to make the v2.0 alpha available to nightly users (deadline for v4 API switchover is the end of this month, and I should probably have people not me testing while they can still fall back to something that works).
• pt_assistant -- ​https://github.com/JOSM/pt_assistant/pull/38
• shapetools -- ​https://github.com/JOSM/ShapeTools/pull/5
• trustosm -- note: this doesn't appear to be a currently published plugin. I've updated the build.xml, but I haven't published the plugin.

Note: I don't have svn access, so someone else will have to rebuild to get the "right" revision in the jar files, for svn managed plugins.

[stoecker]

Note: I don't have svn access, so someone else will have to rebuild to get the "right" revision in the jar files, for svn managed plugins.

I gave you access to plugins SVN.

[taylor.smock]

Replying to stoecker:

I gave you access to plugins SVN.

Thanks. Let me know if I'm doing something wrong (I'm following https://josm.openstreetmap.de/wiki/DevelopersGuide/DevelopingPlugins#Publishingthenewplugin for updating plugins, since it seems to also apply to updates).

[stoecker]

Replying to taylor.smock:

Replying to stoecker:

I gave you access to plugins SVN.

Thanks. Let me know if I'm doing something wrong (I'm following https://josm.openstreetmap.de/wiki/DevelopersGuide/DevelopingPlugins#Publishingthenewplugin for updating plugins, since it seems to also apply to updates).

You only need to remember that's a two stage process: First do all source changes, checkin. Do an "svn up" and then build and checkin binary. And also binary must be compiled with Java 8 ;-)

[taylor.smock]

OK. All JOSM SVN plugins have been updated, and I've made pull requests for the plugins in the GitHub JOSM repo (edit: see links in comment:45).

I've also made a PR for the Contour Merge plugin ( ​https://github.com/Gubaer/josm-contourmerge-plugin/pull/25 , see #20882 ). Hopefully I got the right repo. :)

[simon04]

For the reference, [35744:35752/osm].

It's probably too late now, but since we're recompiling all plugins, we could drop the "binary compatibility" overrides introduced in r17862 and make the public fields private and use proper getters?

[taylor.smock]

@simon04: I don't mind dropping the binary compatibility overrides and recompiling the plugins (again). I'll just go back and bump the PR's for the GitHub plugins.

Also, I'm running the following script to make certain I don't miss any of the plugins (note: I'm probably going to have to modify this):

#!/usr/bin/env bash
set -ex

function get_plugins() {
  curl -O https://josm.openstreetmap.de/plugin
  # Get the plugin download URLs
  JOBS=(`cat plugin | grep -E "^.*.jar;" | awk -F';' '{print $2}'`)
  mkdir -p .cache
  # This just caches the plugins, so that a rerun doesn't get the plugins again
  for PLUGIN in $JOBS; do
    echo $PLUGIN
    PLUGIN_NAME="${PLUGIN##*/}"
    curl -Lz .cache/$PLUGIN_NAME --output .cache/$PLUGIN_NAME $PLUGIN
  done
}

function get_josm() {
  # Check if already downloaded
  if [ -e ".cache/josm-r$1.jar" ]; then
    return
  fi
  if [[ $1 =~ "^[0-9]+$" ]]; then
      curl -Lz .cache/josm-r$1.jar --output .cache/josm-r$1.jar https://josm.openstreetmap.de/download/josm-snapshot-$1.jar
    if [[ ! -e ".cache/josm-r$1.jar" ]]; then
      curl -Lz .cache/josm-r$1.jar --output .cache/josm-r$1.jar https://josm.openstreetmap.de/download/Archiv/josm-snapshot-$1.jar
    fi
  else
    curl -Lz .cache/josm-$1.jar --output .cache/josm-$1.jar https://josm.openstreetmap.de/download/josm-$1.jar
  fi
}

function run_checker() {
  get_josm $1
  get_josm $2
  for i in .cache/*; do
    while [ $(jobs | wc -l) -gt 4 ]; do
      sleep 10s
    done
    NAME=${i#.cache/}
    docker run -v $(pwd):/app --rm tsmock77/japi-compliance-checker:latest japi-compliance-checker --lib=JOSM .cache/josm-r$1.jar --v1=$1 .cache/josm-r$2.jar --v2=$2 -client $i -report-path compat_reports/JOSM/$1_to_$2/${NAME%.jar}.html &
  done
}

function main() {
  get_plugins
  # Note: These were manually built and placed in `.cache`, as neither can be downloaded.
  run_checker 17861 17862
}

main $@

[simon04]

In 17896/josm:

see #17177 - IWaySegment: encapsulate fields

[simon04]

In 17897/josm:

see #17177 - WaySegment: remove binary compatibility overrides

[taylor.smock]

r17896 is going to be a bit more difficult than r17897 (r17896 is going to require more source code modification, beyond a simple recompile, but probably a good idea anyway).

I'll try to get all the patches out for it today.

[taylor.smock]

@simon04: Quick question on r17896: In IWaySegment#getUpperIndex, did you intend the following:

    public int getUpperIndex() {
        return lowerIndex;
    }

or did you intend to write

    public int getUpperIndex() {
        return lowerIndex + 1;
    }

[simon04]

Replying to taylor.smock:

@simon04: Quick question on r17896: In IWaySegment#getUpperIndex, did you intend the following:

Yes, of course. Thanks for spotting this glitch.

[simon04]

In 17902/josm:

see #17177 - Encapsulate IWaySegment fields (fix getUpperIndex)

[taylor.smock]

Replying to simon04:

Yes, of course. Thanks for spotting this glitch.

No problem. I'm porting AlignWays right now, and it had some WaySegment.lowerIndex + 1 in if statements, and I figured that I'd check that the logic for getUpperIndex was the same prior to using it.

As an FYI, I'm going to be changing WaySegment to IWaySegment<Node, Way> in the code I'm porting. I figure that it will make it easier to modify anything that returns WaySegment in the future, if needed.

[taylor.smock]

I've updated all the PRs (see comment:45). Someone with write permissions on the JOSM GitHub will need to look at and merge them.

Two specifically need to wait for the nightly (I need to update the compile version, which is downloaded):

• ​https://github.com/JOSM/pt_assistant/pull/38
• ​https://github.com/Gubaer/josm-contourmerge-plugin/pull/25

EDIT:
From the script at comment:51, the following plugins have issues between JOSM r17861 and r17862 (since r17862 pretty much hides way and lowerIndex without a recompile, it also applies to r17896):

• alignways (WaySegment)
• CadTools (WaySegment) -- see ​https://github.com/ROTARIUANAMARIA/CADTools/pull/7 (PR open, non JOSM)
• contourmerge (WaySegment) -- see comment:3:ticket:20882 (PR open, non JOSM)
• CustomizePublicTransportStop (WaySegment)
• FixAddresses (RelationMember#getWay -- return type Way -> IWay)
• graphview (RelationMember#getWay)
• imagery-xml-bounds (RelationMember#getWay)
• ImproveWay (WaySegment)
• intersection (RelationMember#getWay)
• Mapillary (WaySegment, FilterWorker)
• mapwithai (WaySegment, RelationMember#getWay)
• PolygonCutOut (RelationMember#getWay)
• pt_assistant (WaySegment, RelationMember#getWay)
• reltoolbox (RelationMember#getWay)
• turnrestrictions (RelationMember#getWay)
• turnlanes (RelationMember#getWay)
• scripting (RelationMember#getWay)
• ShapeTools (WaySegment)
• TombPlugin (RelationMember#getWay)

All 160 plugins have been checked. I haven't seen any crashes with RelationMember#getWay (IIRC I checked with graphview specifically), so I don't think I have to recompile for those.

[simon04]

Replying to taylor.smock:

Affected plugins (will be updated as I go through them):

Thank you for updating the plugins! I've merged the PR and released the following versions:

• ​https://github.com/JOSM/auto-tools/releases/tag/v.1.3.3
• ​https://github.com/JOSM/Building-Generalization/releases/tag/v1.1
• ​https://github.com/JOSM/improve-way/releases/tag/v1.5.3
• ​https://github.com/JOSM/pt_assistant/releases/tag/v2.1.15
• ​https://github.com/JOSM/ShapeTools/releases/tag/v1.2.5

PluginsSource?action=diff&version=615

[taylor.smock]

Replying to simon04:

Thank you for updating the plugins! I've merged the PR and released the following versions:

No problem. It was kind of fallout from some of my coding decisions.

All plugins that were affected by r17896 have patches available or applied.

All JOSM controlled plugins have been updated. Non JOSM controlled plugins will be updated as the authors do the updating.

[taylor.smock]

Replying to simon04:

Thank you for updating the plugins! I've merged the PR and released the following versions:

• ​https://github.com/JOSM/auto-tools/releases/tag/v.1.3.3

PluginsSource?action=diff&version=615

Just an FYI, I changed the URL for auto-tools from https://github.com/JOSM/auto-tools/releases/download/v1.3.3/auto_tools.jar to https://github.com/JOSM/auto-tools/releases/download/v.1.3.3/auto_tools.jar -- it looks like GitHub doesn't change the download links when the release name is changed.
PluginsSource?action=diff&version=619

[simon04]

Two unit tests are broken on Windows – ​https://github.com/openstreetmap/josm/runs/2731620532?check_suite_focus=true#step:8:15

Test: testMapillaryStyle() took 411 milli sec(s) FAILED: Illegal char <:> at index 4: file:\D:\a\josm\josm\test/data/\mapillary.json
java.nio.file.InvalidPathException: Illegal char <:> at index 4: file:\D:\a\josm\josm\test/data/\mapillary.json
	at java.base/java.nio.file.Path.of(Path.java:147)
	at java.base/java.nio.file.Paths.get(Paths.java:69)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.style.MapboxVectorStyleTest.testMapillaryStyle(MapboxVectorStyleTest.java:240)

Test: testSprites() took 434 milli sec(s) FAILED: Unexpected char 85 at (line no=1, column no=92, offset=91)
javax.json.stream.JsonParsingException: Unexpected char 85 at (line no=1, column no=92, offset=91)
	at org.glassfish.json.JsonTokenizer.unexpectedChar(JsonTokenizer.java:601)
	at org.glassfish.json.JsonTokenizer.unescape(JsonTokenizer.java:231)
	at org.glassfish.json.JsonTokenizer.readString(JsonTokenizer.java:184)
	at org.glassfish.json.JsonTokenizer.nextToken(JsonTokenizer.java:379)
	at org.glassfish.json.JsonParserImpl$ObjectContext.getNextEvent(JsonParserImpl.java:481)
	at org.glassfish.json.JsonParserImpl.next(JsonParserImpl.java:376)
	at org.glassfish.json.JsonParserImpl.getObject(JsonParserImpl.java:340)
	at org.glassfish.json.JsonParserImpl.getObject(JsonParserImpl.java:173)
	at org.glassfish.json.JsonReaderImpl.read(JsonReaderImpl.java:94)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.style.MapboxVectorStyleTest.getJson(MapboxVectorStyleTest.java:229)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.style.MapboxVectorStyleTest.checkImages(MapboxVectorStyleTest.java:165)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.style.MapboxVectorStyleTest.testSprites(MapboxVectorStyleTest.java:154)

[taylor.smock]

Awesome. I don't have a Windows system handy, so I'll have to blindly guess and hope that I've fixed the problem. Probably a / versus \ problem.

[taylor.smock]

Use File.separator

[taylor.smock]

Avoid using methods which use File.separator instead of /

[taylor.smock]

Replying to simon04:

Two unit tests are broken on Windows – ​https://github.com/openstreetmap/josm/runs/2731620532?check_suite_focus=true#step:8:15

I suspect that attachment:17177.windows_test_fix_2.patch​ is the correct one, since TestUtils.getTestDataRoot() uses a hard coded /.

[Don-vip]

In 17960/josm:

see #17177 - fix two unit tests broken on Windows (patch by taylor.smock)

[Don-vip]

Congrats Taylor, this is awesome work! I unfortunately couldn't review your work at all in the past months, I'm glad Simon did it :) It's really great to have an embedded protobuf parser in JOSM core. I'll take a look to reuse it in my PBF plugin instead of the official protobuf library so that it can be merged into core as well.

[taylor.smock]

I blame Mapillary. :)

It was actually kind of interesting to work on a the parser.

I'm also expecting to have to fix some edge cases (there's one right now which involves whether or not 0 is positive, but I've been busy tweaking the Mapillary plugin). And now I'm on vacation, so I'm only trying to fix critical bugs.

[Don-vip]

OK. The unit tests are still failing on Windows, I'll take a look ​https://github.com/openstreetmap/josm/actions/runs/1009839109

[taylor.smock]

Looking at it again,
https://josm.openstreetmap.de/browser/josm/trunk/test/unit/org/openstreetmap/josm/data/imagery/vectortile/mapbox/style/MapboxVectorStyleTest.java?annotate=blame&rev=17960#L65
and
https://josm.openstreetmap.de/browser/josm/trunk/test/unit/org/openstreetmap/josm/data/imagery/vectortile/mapbox/style/MapboxVectorStyleTest.java?annotate=blame&rev=17960#L164

Probably doing a .replace(File.separator, '/') on L165 would do the trick.

[Don-vip]

In 17961/josm:

see #17177 - fix unit test on Windows

[Don-vip]

In 17965/josm:

see #17177 - unit test on Windows VM still failing - remove space from test folder name

[Don-vip]

Last error to fix on Github actions for this test:

java.io.FileNotFoundException: C:\Users\RUNNER~1\AppData\Local\Temp\junit8759771515663820044\junit8837891059388623545\images\test_style\(0,0).png (Access is denied)
	at java.io.RandomAccessFile.open0(Native Method)
	at java.io.RandomAccessFile.open(RandomAccessFile.java:316)
	at java.io.RandomAccessFile.<init>(RandomAccessFile.java:243)
	at javax.imageio.stream.FileImageOutputStream.<init>(FileImageOutputStream.java:69)
	at com.sun.imageio.spi.FileImageOutputStreamSpi.createOutputStreamInstance(FileImageOutputStreamSpi.java:55)
	at javax.imageio.ImageIO.createImageOutputStream(ImageIO.java:419)
	at javax.imageio.ImageIO.write(ImageIO.java:1543)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.style.MapboxVectorStyle.save(MapboxVectorStyle.java:238)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.style.MapboxVectorStyle.parseSprites(MapboxVectorStyle.java:201)
	at org.openstreetmap.josm.data.imagery.vectortile.mapbox.style.MapboxVectorStyle.fetchSprites(MapboxVectorStyle.java:168)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

[Don-vip]

In 17968/josm:

see #17177 - don't create unneeded output stream when writing PNG files - might be the cause of unit test failure on Github windows runner

[Don-vip]

In 17990/josm:

see #17177 - detailed errors for random unit test failures

[Don-vip]

The ticket is fixed, but did anyone add back OpenInfraMap to our list of default maps? Can't find it on Maps/Worldwide

[Don-vip]

In 17994/josm:

see #17177 - see #20971 - MVT robustness, better logs, javadoc

[skyper]

Replying to Don-vip:

The ticket is fixed, but did anyone add back OpenInfraMap to our list of default maps? Can't find it on Maps/Worldwide

I guess, this was forgotten.

[Don-vip]

In 18011/josm:

see #17177 - add debug log on saving, wait for saving in testMapillaryStyle, code style

[taylor.smock]

Replying to skyper:

Replying to Don-vip:

The ticket is fixed, but did anyone add back OpenInfraMap to our list of default maps? Can't find it on Maps/Worldwide

I guess, this was forgotten.

I was intending to do this. I was going to see if I could find the contact info for the operator, so we could get a usable style sheet.

[taylor.smock]

Replying to Don-vip:

In 17968/josm:

see #17177 - don't create unneeded output stream when writing PNG files - might be the cause of unit test failure on Github windows runner

I was specifically checking that the PNG files were written, since other sections of the code might have surprising behavior if that doesn't happen (e.g., icons don't show up properly). Not necessarily an issue, as long as we know that we aren't actually testing that behavior.

[Don-vip]

Replying to taylor.smock:

I was intending to do this. I was going to see if I could find the contact info for the operator, so we could get a usable style sheet.

He can be reached on Twitter:
​https://twitter.com/openinframap
​https://twitter.com/russss

[Don-vip]

You can probably find the style sheet there? ​https://github.com/openinframap/styles

[taylor.smock]

Replying to Don-vip:

Replying to taylor.smock:

I was intending to do this. I was going to see if I could find the contact info for the operator, so we could get a usable style sheet.

He can be reached on Twitter:
​https://twitter.com/openinframap
​https://twitter.com/russss

I'd have to see if I can find someone with a twitter account (I don't have one). But it does look like he is on IRC, so I'll just ping him there.

Replying to Don-vip:

You can probably find the style sheet there? ​https://github.com/openinframap/styles

Nope.
​https://github.com/openinframap/web/tree/master/src/style is where it lives. There is a style.json file, but it doesn't store actual style information -- that is all in the javascript files.

[taylor.smock]

Well, I pinged russss on IRC, and he said that he only had it in javascript so that it could be deduplicated.

What we can do (short term) is just readd the raw URL and call it good.

Longer term, we should probably look into writing a JS parser that will spit out the style sheet.

[Klumbumbus]

In 18086/josm:

see #17177, see #21143 - unify icon with the other 3

[taylor.smock]

Replying to taylor.smock:

Well, I pinged russss on IRC, and he said that he only had it in javascript so that it could be deduplicated.

What we can do (short term) is just readd the raw URL and call it good.

Longer term, we should probably look into writing a JS parser that will spit out the style sheet.

Quick update: I've added OpenInfraMap back to the list in https://josm.openstreetmap.de/wiki/Maps/Worldwide?action=diff&version=122&old_version=121

I'm opening another ticket on where to put stylesheets for MVT layers. See #21194.

[Klumbumbus]

Are there special things to add to the docu for mvt similar to what we have for wms/tms/wmts? -> wiki:/Maps#TileMapServicesTMS

[taylor.smock]

Replying to Klumbumbus:

Are there special things to add to the docu for mvt similar to what we have for wms/tms/wmts? -> wiki:/Maps#TileMapServicesTMS

Maybe. About the only special thing to add is that the link can be to a Mapbox Vector Style document, and if the style has zoom information for the sources, it isn't necessary to add the zoom information to the JOSM entry.

We might want to note that we currently want to have the vector style documents in JOSM plugin SVN (see #21194), but it isn't required.

[Klumbumbus]

Could you please add the information there?

[taylor.smock]

Replying to Klumbumbus:

Could you please add the information there?

See https://josm.openstreetmap.de/wiki/Maps#MapboxVectorTileServicesMVT

Hopefully it is clearer than mud. :)

[Don-vip]

In 18214/josm:

see #17177 - fix potential NPE (coverity 1458165)