Modify

Opened 3 years ago

Closed 13 months ago

Last modified 8 months 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 16 months 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 15 months ago.
Initial rendering work (currently only works on zoom level 14)
17177.2.patch (82.9 KB) - added by taylor.smock 15 months ago.
Rendering works on all zoom levels (quirks are present at low zoom levels)
17177.3.patch (103.8 KB) - added by taylor.smock 15 months 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 15 months 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 14 months 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 14 months ago.
Styling is used now, icons need some additional special handling (not yet implemented)
pbf.tar.xz (267.7 KB) - added by taylor.smock 14 months ago.
PBFs used for testing
17177.7.protobuf.patch (62.3 KB) - added by taylor.smock 14 months 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 14 months 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 13 months ago.
17177.0001-MVT-Update-maps.xsd-for-mvt-support.patch (1.6 KB) - added by taylor.smock 13 months ago.
Add MVT to maps.xsd
17177-r17862.patch (685.4 KB) - added by simon04 13 months ago.
17177.binary_compat.patch (1.1 KB) - added by taylor.smock 12 months ago.
Binary compatibility
17177.windows_test_fix_1.patch (1.2 KB) - added by taylor.smock 12 months ago.
Use File.separator
17177.windows_test_fix_2.patch (1.5 KB) - added by taylor.smock 12 months ago.
Avoid using methods which use File.separator instead of /

Change History (107)

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

comment:2 Changed 3 years ago by Don-vip

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

comment:3 Changed 19 months ago by 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.

Last edited 19 months ago by pyrog (previous) (diff)

comment:4 Changed 16 months ago by taylor.smock

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)

comment:5 in reply to:  4 ; Changed 16 months ago by 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.

comment:6 in reply to:  5 ; Changed 16 months ago by taylor.smock

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:

Changed 16 months ago by taylor.smock

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)

comment:7 in reply to:  6 ; Changed 16 months ago by 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 ;-)

Changed 15 months ago by taylor.smock

Attachment: 17177.1.patch added

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

comment:8 in reply to:  7 Changed 15 months ago by 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).

Changed 15 months ago by taylor.smock

Attachment: 17177.2.patch added

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

comment:9 Changed 15 months ago by 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)

Test layers:

Last edited 15 months ago by taylor.smock (previous) (diff)

Changed 15 months ago by taylor.smock

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 Changed 15 months ago by stoecker

Milestone: 21.02

comment:11 Changed 15 months ago by Don-vip

Milestone: 21.0221.03
Priority: normalmajor

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

comment:12 Changed 15 months ago by 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

Changed 15 months ago by taylor.smock

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

Changed 14 months ago by taylor.smock

Attachment: 17177.5.patch added

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

Changed 14 months ago by taylor.smock

Attachment: 17177.6.patch added

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

comment:14 in reply to:  13 Changed 14 months ago by 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)).

comment:15 Changed 14 months ago by simon04

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

comment:16 Changed 14 months ago by 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.

Changed 14 months ago by taylor.smock

Attachment: pbf.tar.xz added

PBFs used for testing

comment:17 Changed 14 months ago by 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.

Last edited 14 months ago by taylor.smock (previous) (diff)

Changed 14 months ago by taylor.smock

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 Changed 14 months ago by Don-vip

Milestone: 21.0321.04

Changed 14 months ago by taylor.smock

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 Changed 14 months ago by 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)

comment:20 Changed 13 months ago by 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).

Changed 13 months ago by taylor.smock

Attachment: 17177.9.patch added

comment:21 Changed 13 months ago by taylor.smock

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 Changed 13 months ago by simon04

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?

comment:23 in reply to:  22 Changed 13 months ago by 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.

comment:25 in reply to:  20 ; Changed 13 months ago by 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 :-(

comment:26 Changed 13 months ago by 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!

Last edited 13 months ago by stoecker (previous) (diff)

Changed 13 months ago by taylor.smock

Add MVT to maps.xsd

comment:27 in reply to:  25 ; Changed 13 months ago by 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.

comment:28 in reply to:  27 Changed 13 months ago by 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 🙄

comment:29 in reply to:  22 Changed 13 months ago by 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.

comment:30 Changed 13 months ago by simon04

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

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

comment:32 Changed 13 months ago by 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?

comment:33 in reply to:  32 Changed 13 months ago by 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.

comment:34 Changed 13 months ago by 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).

comment:35 Changed 13 months ago by simon04

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

Changed 13 months ago by simon04

Attachment: 17177-r17862.patch added

comment:36 Changed 13 months ago by simon04

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

comment:37 in reply to:  36 Changed 13 months ago by 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).

comment:38 Changed 13 months ago by simon04

In 17867/josm:

see #17177 - fix @since xxx

comment:39 Changed 12 months ago by simon04

In 17879/josm:

see #17177 - Fix LayersTest.testSymbol

comment:40 Changed 12 months ago by 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 acronyms), then the second url should be written the same way.

Last edited 12 months ago by holgermappt (previous) (diff)

comment:41 Changed 12 months ago by 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.

Last edited 12 months ago by taylor.smock (previous) (diff)

comment:42 in reply to:  41 Changed 12 months ago by 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.

comment:43 Changed 12 months ago by simon04

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

comment:44 in reply to:  43 Changed 12 months ago by taylor.smock

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 12 months ago by taylor.smock (previous) (diff)

Changed 12 months ago by taylor.smock

Attachment: 17177.binary_compat.patch added

Binary compatibility

comment:45 Changed 12 months ago by taylor.smock

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 12 months ago by taylor.smock (previous) (diff)

comment:46 in reply to:  45 ; Changed 12 months ago by 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.

comment:47 in reply to:  46 ; Changed 12 months ago by 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).

comment:48 in reply to:  47 Changed 12 months ago by 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 ;-)

comment:49 Changed 12 months ago by 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. :)

Last edited 12 months ago by taylor.smock (previous) (diff)

comment:50 Changed 12 months ago by 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?

comment:51 Changed 12 months ago by 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 $@
Last edited 12 months ago by taylor.smock (previous) (diff)

comment:52 Changed 12 months ago by simon04

In 17896/josm:

see #17177 - IWaySegment: encapsulate fields

comment:53 Changed 12 months ago by simon04

In 17897/josm:

see #17177 - WaySegment: remove binary compatibility overrides

comment:54 Changed 12 months ago by 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.

comment:55 Changed 12 months ago by 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;
    }
Last edited 12 months ago by taylor.smock (previous) (diff)

comment:56 in reply to:  55 ; Changed 12 months ago by 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.

comment:57 Changed 12 months ago by simon04

In 17902/josm:

see #17177 - Encapsulate IWaySegment fields (fix getUpperIndex)

comment:58 in reply to:  56 Changed 12 months ago by 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.

comment:59 Changed 12 months ago by 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):

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 12 months ago by taylor.smock (previous) (diff)

comment:61 in reply to:  60 Changed 12 months ago by 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.

comment:62 in reply to:  60 Changed 12 months ago by taylor.smock

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 Changed 12 months ago by 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)

comment:64 Changed 12 months ago by 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.

Changed 12 months ago by taylor.smock

Use File.separator

Changed 12 months ago by taylor.smock

Avoid using methods which use File.separator instead of /

comment:65 in reply to:  63 Changed 12 months ago by 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 /.

comment:66 Changed 11 months ago by Don-vip

In 17960/josm:

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

comment:67 Changed 11 months ago by 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.

comment:68 in reply to:  67 Changed 11 months ago by 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.

Last edited 11 months ago by taylor.smock (previous) (diff)

comment:69 Changed 10 months ago by Don-vip

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

comment:71 Changed 10 months ago by Don-vip

In 17961/josm:

see #17177 - fix unit test on Windows

comment:72 Changed 10 months ago by Don-vip

In 17965/josm:

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

comment:73 Changed 10 months ago by 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)
Last edited 10 months ago by Don-vip (previous) (diff)

comment:74 Changed 10 months ago by 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

comment:75 Changed 10 months ago by Don-vip

In 17990/josm:

see #17177 - detailed errors for random unit test failures

comment:76 Changed 10 months ago by 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

comment:77 Changed 10 months ago by Don-vip

In 17994/josm:

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

comment:78 in reply to:  76 ; Changed 10 months ago by 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.

comment:79 Changed 10 months ago by Don-vip

In 18011/josm:

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

comment:80 in reply to:  78 ; Changed 10 months ago by 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.

comment:81 in reply to:  74 Changed 10 months ago by 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.

comment:82 in reply to:  80 ; Changed 10 months ago by 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

comment:83 Changed 10 months ago by Don-vip

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

comment:84 in reply to:  82 Changed 10 months ago by 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.

Last edited 10 months ago by taylor.smock (previous) (diff)

comment:85 in reply to:  82 ; Changed 10 months ago by 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.

comment:86 Changed 10 months ago by Klumbumbus

In 18086/josm:

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

comment:87 in reply to:  85 Changed 10 months ago by 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.

comment:88 Changed 10 months ago by Klumbumbus

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

comment:89 in reply to:  88 Changed 10 months ago by 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.

comment:90 Changed 10 months ago by Klumbumbus

Could you please add the information there?

comment:91 in reply to:  90 Changed 10 months ago by 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. :)

comment:92 Changed 8 months ago by Don-vip

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.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.