Modify

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#17177 closed enhancement (fixed)

[WIP PATCH] Add support for Mapbox Vector Tile

Reported by: Don-vip Owned by: simon04
Priority: major Milestone: 21.05
Component: Core imagery Version:
Keywords: mvt mapbox vector tile Cc: Don-vip, stoecker, GerdP

Description

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:

    <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>

Attachments (16)

17177.patch (37.9 KB ) - added by taylor.smock 3 years ago.
WIP Patch. DO NOT APPLY. Parsing of Mapbox Vector Tiles partially works. Packed fields do not yet work (specifically, tags and geometry)
17177.1.patch (77.9 KB ) - added by taylor.smock 3 years ago.
Initial rendering work (currently only works on zoom level 14)
17177.2.patch (82.9 KB ) - added by taylor.smock 3 years ago.
Rendering works on all zoom levels (quirks are present at low zoom levels)
17177.3.patch (103.8 KB ) - added by taylor.smock 3 years ago.
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).
17177.4.patch (170.9 KB ) - added by taylor.smock 3 years ago.
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).
17177.5.patch (246.1 KB ) - added by taylor.smock 3 years ago.
Checkpoint -- style generation should be good enough, although it is not yet currently used
17177.6.patch (273.5 KB ) - added by taylor.smock 3 years ago.
Styling is used now, icons need some additional special handling (not yet implemented)
pbf.tar.xz (267.7 KB ) - added by taylor.smock 3 years ago.
PBFs used for testing
17177.7.protobuf.patch (62.3 KB ) - added by taylor.smock 3 years ago.
Protobuf only classes and tests (tests cover most everything, including most mutations). Files needed for tests will be uploaded separately.
17177.8.patch (356.7 KB ) - added by taylor.smock 3 years ago.
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)
17177.9.patch (370.7 KB ) - added by taylor.smock 3 years ago.
17177.0001-MVT-Update-maps.xsd-for-mvt-support.patch (1.6 KB ) - added by taylor.smock 3 years ago.
Add MVT to maps.xsd
17177-r17862.patch (685.4 KB ) - added by simon04 3 years ago.
17177.binary_compat.patch (1.1 KB ) - added by taylor.smock 3 years ago.
Binary compatibility
17177.windows_test_fix_1.patch (1.2 KB ) - added by taylor.smock 3 years ago.
Use File.separator
17177.windows_test_fix_2.patch (1.5 KB ) - added by taylor.smock 3 years ago.
Avoid using methods which use File.separator instead of /

Change History (107)

comment:1 by GerdP, 5 years ago

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

comment:2 by Don-vip, 5 years ago

Component: Plugin pbfCore imagery
Owner: changed from Don-vip to team

comment:3 by pyrog, 3 years ago

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.

Last edited 3 years ago by pyrog (previous) (diff)

comment:4 by taylor.smock, 3 years ago

Summary: Add support for Mapbox Vector Tile[RFC] Add support for Mapbox Vector Tile

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)

in reply to:  4 ; comment:5 by stoecker, 3 years ago

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.

in reply to:  5 ; comment:6 by taylor.smock, 3 years ago

Summary: [RFC] Add support for Mapbox Vector Tile[WIP PATCH] Add support for Mapbox Vector Tile

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:

by taylor.smock, 3 years ago

Attachment: 17177.patch added

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

in reply to:  6 ; comment:7 by stoecker, 3 years ago

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 ;-)

by taylor.smock, 3 years ago

Attachment: 17177.1.patch added

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

in reply to:  7 comment:8 by taylor.smock, 3 years ago

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).

by taylor.smock, 3 years ago

Attachment: 17177.2.patch added

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

comment:9 by taylor.smock, 3 years ago

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)

Test layers:

Last edited 3 years ago by taylor.smock (previous) (diff)

by taylor.smock, 3 years ago

Attachment: 17177.3.patch added

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).

comment:10 by stoecker, 3 years ago

Milestone: 21.02

comment:11 by Don-vip, 3 years ago

Milestone: 21.0221.03
Priority: normalmajor

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

comment:12 by taylor.smock, 3 years ago

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

by taylor.smock, 3 years ago

Attachment: 17177.4.patch added

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).

by taylor.smock, 3 years ago

Attachment: 17177.5.patch added

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

by taylor.smock, 3 years ago

Attachment: 17177.6.patch added

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

in reply to:  13 comment:14 by taylor.smock, 3 years ago

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)).

comment:15 by simon04, 3 years ago

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

comment:16 by taylor.smock, 3 years ago

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.

by taylor.smock, 3 years ago

Attachment: pbf.tar.xz added

PBFs used for testing

comment:17 by taylor.smock, 3 years ago

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

by taylor.smock, 3 years ago

Attachment: 17177.7.protobuf.patch added

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

comment:18 by Don-vip, 3 years ago

Milestone: 21.0321.04

by taylor.smock, 3 years ago

Attachment: 17177.8.patch added

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)

comment:19 by taylor.smock, 3 years ago

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)

comment:20 by taylor.smock, 3 years ago

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).

by taylor.smock, 3 years ago

Attachment: 17177.9.patch added

comment:21 by taylor.smock, 3 years ago

https://matrix-client.matrix.org/_matrix/media/r0/download/matrix.org/DUmVrDREaiWZCOgfVFlYpoRh

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.

comment:22 by simon04, 3 years ago

Cc: Don-vip stoecker GerdP added

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?

in reply to:  22 comment:23 by taylor.smock, 3 years ago

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.

in reply to:  20 ; comment:25 by stoecker, 3 years ago

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 :-(

comment:26 by stoecker, 3 years ago

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!

Last edited 3 years ago by stoecker (previous) (diff)

by taylor.smock, 3 years ago

Add MVT to maps.xsd

in reply to:  25 ; comment:27 by taylor.smock, 3 years ago

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.

in reply to:  27 comment:28 by stoecker, 3 years ago

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 🙄

in reply to:  22 comment:29 by taylor.smock, 3 years ago

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.

comment:30 by simon04, 3 years ago

Milestone: 21.0421.05
Owner: changed from team to simon04
Status: newassigned

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

in reply to:  30 comment:31 by taylor.smock, 3 years ago

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.

comment:32 by simon04, 3 years ago

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?

in reply to:  32 comment:33 by taylor.smock, 3 years ago

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.

comment:34 by taylor.smock, 3 years ago

@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).

comment:35 by simon04, 3 years ago

Resolution: fixed
Status: assignedclosed

In 17862/josm:

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

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

by simon04, 3 years ago

Attachment: 17177-r17862.patch added

comment:36 by simon04, 3 years ago

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

in reply to:  36 comment:37 by taylor.smock, 3 years ago

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).

comment:38 by simon04, 3 years ago

In 17867/josm:

see #17177 - fix @since xxx

comment:39 by simon04, 3 years ago

In 17879/josm:

see #17177 - Fix LayersTest.testSymbol

comment:40 by holgermappt, 3 years ago

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 acronyms), then the second url should be written the same way.

Last edited 3 years ago by holgermappt (previous) (diff)

comment:41 by taylor.smock, 3 years ago

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

in reply to:  41 comment:42 by holgermappt, 3 years ago

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.

comment:43 by simon04, 3 years ago

Relevant commit regarding i18 strings: r13591

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

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

in reply to:  43 comment:44 by taylor.smock, 3 years ago

Replying to simon04:

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

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

by taylor.smock, 3 years ago

Attachment: 17177.binary_compat.patch added

Binary compatibility

comment:45 by taylor.smock, 3 years ago

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

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

in reply to:  45 ; comment:46 by stoecker, 3 years ago

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.

in reply to:  46 ; comment:47 by taylor.smock, 3 years ago

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).

in reply to:  47 comment:48 by stoecker, 3 years ago

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 ;-)

comment:49 by taylor.smock, 3 years ago

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. :)

Last edited 3 years ago by taylor.smock (previous) (diff)

comment:50 by simon04, 3 years ago

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?

comment:51 by taylor.smock, 3 years ago

@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 $@
Last edited 3 years ago by taylor.smock (previous) (diff)

comment:52 by simon04, 3 years ago

In 17896/josm:

see #17177 - IWaySegment: encapsulate fields

comment:53 by simon04, 3 years ago

In 17897/josm:

see #17177 - WaySegment: remove binary compatibility overrides

comment:54 by taylor.smock, 3 years ago

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.

comment:55 by taylor.smock, 3 years ago

@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;
    }
Last edited 3 years ago by taylor.smock (previous) (diff)

in reply to:  55 ; comment:56 by simon04, 3 years ago

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.

comment:57 by simon04, 3 years ago

In 17902/josm:

see #17177 - Encapsulate IWaySegment fields (fix getUpperIndex)

in reply to:  56 comment:58 by taylor.smock, 3 years ago

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.

comment:59 by taylor.smock, 3 years ago

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):

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

in reply to:  60 comment:61 by taylor.smock, 3 years ago

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.

in reply to:  60 comment:62 by taylor.smock, 3 years ago

Replying to simon04:

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

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

comment:63 by simon04, 3 years ago

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)

comment:64 by taylor.smock, 3 years ago

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.

by taylor.smock, 3 years ago

Use File.separator

by taylor.smock, 3 years ago

Avoid using methods which use File.separator instead of /

in reply to:  63 comment:65 by taylor.smock, 3 years ago

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 /.

comment:66 by Don-vip, 3 years ago

In 17960/josm:

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

comment:67 by Don-vip, 3 years ago

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.

in reply to:  67 comment:68 by taylor.smock, 3 years ago

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

comment:69 by Don-vip, 3 years ago

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

comment:71 by Don-vip, 3 years ago

In 17961/josm:

see #17177 - fix unit test on Windows

comment:72 by Don-vip, 3 years ago

In 17965/josm:

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

comment:73 by Don-vip, 3 years ago

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)
Last edited 3 years ago by Don-vip (previous) (diff)

comment:74 by Don-vip, 3 years ago

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

comment:75 by Don-vip, 3 years ago

In 17990/josm:

see #17177 - detailed errors for random unit test failures

comment:76 by Don-vip, 3 years ago

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

comment:77 by Don-vip, 3 years ago

In 17994/josm:

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

in reply to:  76 ; comment:78 by skyper, 3 years ago

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.

comment:79 by Don-vip, 3 years ago

In 18011/josm:

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

in reply to:  78 ; comment:80 by taylor.smock, 3 years ago

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.

in reply to:  74 comment:81 by taylor.smock, 3 years ago

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.

in reply to:  80 ; comment:82 by Don-vip, 3 years ago

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

comment:83 by Don-vip, 3 years ago

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

in reply to:  82 comment:84 by taylor.smock, 3 years ago

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

in reply to:  82 ; comment:85 by taylor.smock, 3 years ago

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.

comment:86 by Klumbumbus, 3 years ago

In 18086/josm:

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

in reply to:  85 comment:87 by taylor.smock, 3 years ago

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.

comment:88 by Klumbumbus, 3 years ago

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

in reply to:  88 comment:89 by taylor.smock, 3 years ago

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.

comment:90 by Klumbumbus, 3 years ago

Could you please add the information there?

in reply to:  90 comment:91 by taylor.smock, 3 years ago

Replying to Klumbumbus:

Could you please add the information there?

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

Hopefully it is clearer than mud. :)

comment:92 by Don-vip, 3 years ago

In 18214/josm:

see #17177 - fix potential NPE (coverity 1458165)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.