Modify

Opened 6 years ago

Closed 7 months ago

Last modified 7 months ago

#7427 closed enhancement (fixed)

Support reprojection (warping) of imagery layer

Reported by: bastiK Owned by: bastiK
Priority: major Milestone: 17.04
Component: Core imagery Version:
Keywords: Cc: A_Pirard, wiktorn

Description

It would be nice, if we could reproject tiles from a WMS/TMS layer (and other raster data) to the current projection, so that sources with different projections can be combined.

Attachments (7)

50.png (642.1 KB) - added by A_Pirard 6 years ago.
Displaying Bing under Lambert screen
europe-northafrica_utm35.png (228.9 KB) - added by bastiK 8 months ago.
reference.png (10.7 KB) - added by bastiK 8 months ago.
test-output.png (10.6 KB) - added by bastiK 8 months ago.
test-differences.png (1.4 KB) - added by bastiK 8 months ago.
reprojection.patch (59.4 KB) - added by bastiK 7 months ago.
DimmingTiles.png (1.0 MB) - added by Klumbumbus 7 months ago.

Download all attachments as: .zip

Change History (131)

comment:1 Changed 6 years ago by bastiK

This should be possible with the GeoTools library. It is currently implemented by the ImportImage (JOSM-)plugin. In the plugin, the JAI lib is used, as well. However, as far as I can tell, JAI licenses aren't GPL compatible. According to this thread, it seams, that JAI is not a hard dependency for image warping with GeoTools, but only needed for very large images.

In any case, this would need to go into a plugin as GeoTools is very bloated.

comment:2 Changed 6 years ago by joshdoe

Cc: joshdoe added

comment:3 Changed 6 years ago by stoecker

Sounds like Piclayer and importimage should be joined and this functionality included there?

comment:4 Changed 6 years ago by A_Pirard

Cc: A_Pirard added; joshdoe removed

comment:5 Changed 6 years ago by A_Pirard

Cc: joshdoe added

comment:6 Changed 6 years ago by A_Pirard

This Ticket #7427 says in internals terms what ticket Ticket #5387 says in practical terms.
The subject of Ticket #5387 has been changed to report EPSG:3170 deficiencies.
Thus, my description for it no longer makes sense.
Why not have continued Ticket #5387 and open a new ticket for 3170?

comment:7 Changed 6 years ago by A_Pirard

When the screen projection is set to EPSG:31370 and a Bing layer is opened, JOSM warns that Lambert 31370 is not supported by Bing. However, the layer is displayed and is almost usable. Just that small slits where the background shows through appear between tiles (see 50.png attachment). It looks like a simple vertical stretching (and maybe horizontal shrinking?) would be a very satisfactory solution for that map.
It sounds like, although displaying Lambert, JOSM is sending the correct WGS84 coordinates to Bing which returns the correct tiles but needing a small warp.

And now what if the screen were set to Mercator and we forced JOSM to translate the WGS84 coordinates to the projection of the &SRS parameter before sending a request? Would the display look about the same? Just that without the warping (yet) would be a great event.

I tried our Lambert servers on Mkt. JOSM is sooo more pleasurable to work with !!!

Changed 6 years ago by A_Pirard

Attachment: 50.png added

Displaying Bing under Lambert screen

comment:8 Changed 5 years ago by skyper

Priority: normalmajor

+10

This would solve many problems including all tools which depend on projection and only work with "mercator" right now.

comment:9 Changed 5 years ago by A_Pirard

Would there be any news here?
I just explained a major OSM mapper that this Ticket is the reason why he can't use the server he wants.
How many mappers do turn away?
Thanks for all the good work.

comment:10 in reply to:  9 ; Changed 5 years ago by bastiK

Replying to A_Pirard:

Would there be any news here?
I just explained a major OSM mapper that this Ticket is the reason why he can't use the server he wants.

Care to elaborate? You can always switch to the server projection, this feature is just a nice-to-have when working with multiple sources of different projections.

comment:11 Changed 5 years ago by A_Pirard

This feature is not "nice to have" but necessary when working with several servers using different projections.
This was the reason for opening #5387 which was set to fixed when it is not.
I was repeatedly reading that javascript is too slow to reproject when I stumbled upon this
http://dev.openlayers.org/sandbox/camptocamp/canvas/openlayers/examples/raster-reprojection.html
So, I wondered if anything could be new in the abovURL. Just guessing.
User confusion probably stems from reading "the projection" when there is the projection used in server requests, that of the data returned, that of coordinates in source codes, displayed on screen, input from the keyboard and, maybe most unexpected, that of the screen. Explanations are often understood only by those who have already understood them. For example, a lot of Open Overlays code speaks of converting in to out projections without saying what they're for.

comment:12 in reply to:  10 ; Changed 5 years ago by A_Pirard

Replying to bastiK:

this feature is just a nice-to-have ... different projections

"just" only if you're fast enough to switch between two+ different projection images so that they appear to overlay...
The whole France is badly longing for reprojection, Belgium too, ...?
The French are allowed to use the government's data. Well, almost...

FYI: OSM-talk-fr: Cadastre et reprojection des images
PVI: Same text in FrEnGooglish

C'est déjà galère de devoir changer de projection tout le temps
(mercator pour les images satellite, lambert4 si le cadastre est raster,
lambert9 pour avoir le patelin d'à côté déjà vectorisé) ;

À ce propos ce serait vraiment fantastique si JOSM pouvait automatiquement
reprojeter vers une même projection commune. Merkaartor fait ça et c'est
absolument fantastique pour afficher simultanément Bing et le cadastre en semi-
transparence. C'est vraiment *la* chose qui me manque dans JOSM.

My short translation:
It's a PITA to have to constantly change projection: Mercator, Lambert4, lambert9, [Belgian Lambert72]...
It would truly be fantastic if JOSM could reproject...
Mercator does that and it's absolutely fantastic to display semi-transparently ...
It's really THE missing feature in JOSM.

Is that "elaborating" enough?
Thanks for JOSM !!!

comment:13 in reply to:  12 Changed 5 years ago by bastiK

Replying to A_Pirard:

Is that "elaborating" enough?

Oui merci, ça suffit.

Still, someone has to take the time and implement it. :)

comment:14 Changed 8 months ago by bastiK

Cc: wiktorn added
Owner: changed from team to bastiK
Status: newassigned

I've got a quick-and-dirty patch to support warping for TMS. Generally, I wouldn't mind starting with a plugin, but it requires so many changes to the core code, that it makes not much sense.

As for an image library, that can deform images according to a custom mathematical formula: I couldn't find any. Geotools warping indeed depends on JAI, namely javax.media.jai.Warp, so nothing we can use. An implementation from scratch shouldn't be too hard.

It also affects the cache, as warping is too slow, to do it on the fly with each paint cycle. The reprojected images are placed in the MemoryTileCache. The original downloaded images are written to disk by the JCS cache, then dropped from memory. The download dialog still displays the tiles in Mercator, so we need a second MemoryTileCache instance for the second projection in use.

comment:15 in reply to:  14 Changed 8 months ago by wiktorn

Replying to bastiK:

The download dialog still displays the tiles in Mercator, so we need a second MemoryTileCache instance for the second projection in use.

We use separate MemoryTileCache instances per all layers. See the comment:
https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/gui/layer/AbstractTileSourceLayer.java#L151

So the download box and imagery layer have already separate instances.

comment:16 Changed 8 months ago by bastiK

Thanks for the hint, this makes it much simpler.

comment:17 in reply to:  14 Changed 8 months ago by Don-vip

Replying to bastiK:

I've got a quick-and-dirty patch to support warping for TMS.

Wow, great !

As for an image library, that can deform images according to a custom mathematical formula: I couldn't find any. Geotools warping indeed depends on JAI, namely javax.media.jai.Warp, so nothing we can use. An implementation from scratch shouldn't be too hard.

If I understand correctly, your current patch relies on GeoTools but it should be doable to get rid of it?

comment:18 Changed 8 months ago by bastiK

It already uses a very simple custom warp algorithm. The quality is so-so, but it should improve with bilinear or bicubic interpolation.

comment:19 Changed 8 months ago by Don-vip

OK. Did you look at jt-warp? It's licensed under Apache 2.0.

comment:20 Changed 8 months ago by bastiK

No, I haven't so far. It seems JAI-ext has Oracle JAI as a dependency (see imports). I'll have a look how deep this dependency is and if there is code that can be salvaged.

comment:21 Changed 8 months ago by bastiK

@wiktorn: I couldn't find any code that calls the method TMSCachedTileLoaderJob#getCachedTile(), can it be removed?

comment:22 in reply to:  21 Changed 8 months ago by wiktorn

Replying to bastiK:

@wiktorn: I couldn't find any code that calls the method TMSCachedTileLoaderJob#getCachedTile(), can it be removed?

I guess so. Looks like it got obsoleted?

Looks like src.org.openstreetmap.gui.jmapviewer.interfaces.TileJob.getTile() could also be removed. This will entail update of some plugins though.

comment:23 Changed 8 months ago by Don-vip

Component: CoreCore imagery
Milestone: 17.04

comment:24 Changed 8 months ago by bastiK

Okay, I'll keep that in mind.

Changed 8 months ago by bastiK

comment:25 Changed 8 months ago by bastiK

Another edition of fun with projections:

Mercator Carto tiles reprojected to UTM 35 + rotated 8 degrees. (Notice how on the standard Mercator map, the Kola peninsula is as big as Turkey? It isn't, really...)

comment:26 Changed 8 months ago by bastiK

In the imagery preferences, we have a setting Fade Color and Fade amount. Is this still a useful feature or is it obsoleted by per layer opacity setting? I'm asking because the current Fade implementation doesn't work with warped tiles and would need to be adapted a bit.

comment:27 Changed 8 months ago by stoecker

I'd drop it. Two options for the same feature are usually not that helpful at all.

comment:28 Changed 8 months ago by bastiK

In 11785/josm:

Imagery: remove fade setting (superseded by layer opacity), see #7427

comment:29 Changed 8 months ago by stoecker

We can also drop the 4326/3857 workaround when we have reprojection...

comment:30 in reply to:  29 ; Changed 8 months ago by Don-vip

Replying to stoecker:

the 4326/3857 workaround

What's this?

comment:31 Changed 8 months ago by Don-vip

In 11786/josm:

see #7427 - checkstyle

comment:32 in reply to:  30 Changed 8 months ago by stoecker

Replying to Don-vip:

Replying to stoecker:

the 4326/3857 workaround

What's this?

Maps entry epsg4326to3857Supported.

comment:33 in reply to:  30 Changed 8 months ago by wiktorn

Replying to Don-vip:

Replying to stoecker:

the 4326/3857 workaround

What's this?

Using WMS servers to rescale tiles from 4326 to Mercator. I'll happily remove this logic.

comment:34 Changed 8 months ago by bastiK

In 11788/josm:

make it possible to switch between 2 supported projections for a given WMTS layer (see #7427)

comment:35 Changed 8 months ago by bastiK

In 11790/josm:

WMTS: fix case with multiple tilesets per projection and layer (regression from [11788]), see #7427, see #10623)

comment:36 Changed 8 months ago by Don-vip

In 11806/josm:

see #7427 - checkstyle

comment:37 Changed 8 months ago by bastiK

We have 2 fairly elaborate algorithms to display tiles from other zoom levels when the current tile is still loading or has an error:

  • Tile.loadPlaceholderFromCache
  • AbstractTileSourceLayer.drawInViewArea (loop over otherZooms)

As far as I can tell, both do essentially the same thing. Is there a reason it is done this way?

(Btw., Tile.loadPlaceholderFromCache uses tile index arithmetic that probably doesn't work for general WMTS tile matrix set.)

comment:38 Changed 8 months ago by bastiK

In 11829/josm:

see #7427 - move TileRange class to jmapviewer

comment:39 Changed 8 months ago by bastiK

In [o33206] - [jmapviewer] move TileRange? class to jmapviewer
In [o33207] - see ​#7427 - add interface and class for projected coordinates; add several helper functions to TileSource interface

comment:40 Changed 8 months ago by bastiK

In 11830/josm:

see #7427 - add implementations for new TileSource methods

comment:41 Changed 8 months ago by bastiK

In 11831/josm:

see #7427 - use integer operations on tile index when possible (instead of EastNorth or LatLon rectangles)

comment:42 Changed 8 months ago by bastiK

In [o33208] - [jmapviewer] see ​#7427 - add copy constructor

comment:43 Changed 8 months ago by bastiK

In 11832/josm:

see #7427 - remove the tile.loadPlaceholderFromCache mechanism from AbstractTileSourceLayer

There are 2 systems to display replacement tiles from other zoom levels, when the current
tile is not available (yet). This commit turns off one of these systems and relies fully on
the code in AbstractTileSourceLayer.drawInViewArea / paintTileImages.

comment:44 in reply to:  43 ; Changed 8 months ago by wiktorn

Replying to bastiK:

In 11832/josm:

see #7427 - remove the tile.loadPlaceholderFromCache mechanism from AbstractTileSourceLayer

There are 2 systems to display replacement tiles from other zoom levels, when the current
tile is not available (yet). This commit turns off one of these systems and relies fully on
the code in AbstractTileSourceLayer.drawInViewArea / paintTileImages.

Are there no performance drawbacks (esp. with partially loaded imageries, or on places, where max zoom differs for example in Bing)?

loadPlaceholderFromCache only checks in tileCache (memory, with BufferedImages).

The other thing that drawInViewArea / paintTileImages does not do is to create tile images based on data from different zoom level.

comment:45 in reply to:  44 Changed 8 months ago by bastiK

Replying to wiktorn:

Replying to bastiK:

In 11832/josm:

Are there no performance drawbacks (esp. with partially loaded imageries, or on places, where max zoom differs for example in Bing)?

Can you explain in more detail? There are no significant performance drawbacks, that I'm aware of. What do you mean by max zoom differs?

loadPlaceholderFromCache only checks in tileCache (memory, with BufferedImages).

In drawInViewArea, below the line ts.loadAllTiles(false);, it does exactly the same, i.e. check tileCache for images of different zoom level (no new jobs submitted).

The other thing that drawInViewArea / paintTileImages does not do is to create tile images based on data from different zoom level.

The tiles from different zoom levels are drawn on the fly, which should be about as fast.

comment:46 Changed 8 months ago by wiktorn

I'm worried about missing this logic

                /*
                 * use LazyTask for graphics to avoid evaluation of tmpImage, until we have
                 * something to draw
                 */
                CachedCallable<Graphics2D> graphics = new CachedCallable<>(new Callable<Graphics2D>() {
                    @Override
                    public Graphics2D call() throws Exception {
                        Graphics2D g = (Graphics2D) tmpImage.call().getGraphics();
                        g.setTransform(AffineTransform.getScaleInstance(scale, scale));
                        return g;
                    }
                });

                int paintedTileCount = 0;
                for (int x = 0; x < factor; x++) {
                    for (int y = 0; y < factor; y++) {
                        Tile tile = cache.getTile(source, xtileHigh + x, ytileHigh + y, zoomHigh);
                        if (tile != null && tile.isLoaded()) {
                            paintedTileCount++;
                            tile.paint(graphics.call(), x * source.getTileSize(), y * source.getTileSize());
                        }
                    }
                }
                if (paintedTileCount == factor * factor) {
                    image = tmpImage.call();
                    return;
                }
            }

            int zoomLow = zoom - zoomDiff;
            if (zoomLow >= JMapViewer.MIN_ZOOM) {
                int xtileLow = xtile >> zoomDiff;
                int ytileLow = ytile >> zoomDiff;
                final int factor = 1 << zoomDiff;
                final double scale = factor;
                CachedCallable<Graphics2D> graphics = new CachedCallable<>(new Callable<Graphics2D>() {
                    @Override
                    public Graphics2D call() throws Exception {
                        Graphics2D g = (Graphics2D) tmpImage.call().getGraphics();
                        AffineTransform at = new AffineTransform();
                        int translateX = (xtile % factor) * source.getTileSize();
                        int translateY = (ytile % factor) * source.getTileSize();
                        at.setTransform(scale, 0, 0, scale, -translateX, -translateY);
                        g.setTransform(at);
                        return g;
                    }

                });

                Tile tile = cache.getTile(source, xtileLow, ytileLow, zoomLow);
                if (tile != null && tile.isLoaded()) {
                    tile.paint(graphics.call(), 0, 0);
                    image = tmpImage.call();
                    return;
                }
            }

Check for example area like this:
https://www.openstreetmap.org/#map=19/53.969564628111144/21.554929044008773

Goto from zoom 19 to zoom 12 and compare both versions. This is the place, that I guess, that we are loading tiles from different zoom levels.

comment:47 in reply to:  46 Changed 8 months ago by bastiK

Replying to wiktorn:

I'm worried about missing this logic
[...]

From my understanding, the code in drawInViewArea / paintTileImages reproduces this logic (in a slightly different way but with same outcome).

Check for example area like this:
https://www.openstreetmap.org/#map=19/53.969564628111144/21.554929044008773

Goto from zoom 19 to zoom 12 and compare both versions. This is the place, that I guess, that we are loading tiles from different zoom levels.

I did, but don't know what to look out for. In both version, it shows the loaded tiles from level 19 in the center temporarily, then downloads and displays tiles from the current level.

comment:48 Changed 8 months ago by bastiK

In 11834/josm:

see #7427 - change priorities for temporary replacements from other zoom levels: prefer higher zoom over lower

comment:49 Changed 8 months ago by bastiK

In 11835/josm:

see #7427 - alignment to the pixelgrid for precise tile placement

comment:50 Changed 8 months ago by joshdoe

Cc: joshdoe removed

comment:51 Changed 8 months ago by bastiK

In 11838/josm:

see #7427 - fix unit tests

Changed 8 months ago by bastiK

Attachment: reference.png added

Changed 8 months ago by bastiK

Attachment: test-output.png added

Changed 8 months ago by bastiK

Attachment: test-differences.png added

comment:52 Changed 8 months ago by bastiK

For future reference, as result of [11835], the viewport in MapCSSRendererTest moved by about 0.3 pixel in both x and y:

previous: now: difference:

comment:53 Changed 8 months ago by bastiK

wiktorn, if there is something wrong with [11832] I would appreciate your help to understand what is the problem (and fix it). Right now, I'm still convinced it is fine and will proceed.

comment:54 in reply to:  53 Changed 8 months ago by wiktorn

Replying to bastiK:

wiktorn, if there is something wrong with [11832] I would appreciate your help to understand what is the problem (and fix it). Right now, I'm still convinced it is fine and will proceed.

Please continue. You've convinced me that this is ok. Happy to see that code gone away :-)

I'll take [11839] and give it some testing.

Last edited 8 months ago by wiktorn (previous) (diff)

comment:55 Changed 8 months ago by bastiK

In 11840/josm:

see #7427 - small fix for [11835] + change order in zoomNoUndoTo

comment:56 Changed 8 months ago by bastiK

In [o33209] - [jmapviewer] see ​#7427 - adapt the Tile class a bit, so it can store reprojected tiles.

comment:57 Changed 8 months ago by bastiK

In 11841/josm:

see #7427 - rework the way screen pixel coordinates for tiles are calculated

comment:58 Changed 8 months ago by bastiK

In 11842/josm:

see #7427 - clean up private method parameter list

comment:59 Changed 8 months ago by bastiK

In [o33210] - [jmapviewer] see ​#7427 - remove unnecessary API

comment:60 Changed 8 months ago by bastiK

In 11843/josm:

see #7427 - remove unused code

comment:61 Changed 8 months ago by bastiK

I was a bit reluctant to delete TMSCachedTileLoaderJob#getCachedTile(), as loading a tile from disk cache (without http request) seems like a useful functionality. But if it's not used, it has no place in the codebase, I guess.

Most API breaking changes should be done now.

comment:62 Changed 8 months ago by bastiK

In 11844/josm:

see #7427 - avoid roundtrip conversion EastNorth -> LatLon -> EastNorth

This circumvents LatLon clamping, so may have an impact on display near lon=+/-180 degrees

comment:63 Changed 8 months ago by bastiK

In 11845/josm:

see #7427 - better return type

comment:64 in reply to:  61 Changed 8 months ago by wiktorn

Replying to bastiK:

I was a bit reluctant to delete TMSCachedTileLoaderJob#getCachedTile(), as loading a tile from disk cache (without http request) seems like a useful functionality. But if it's not used, it has no place in the codebase, I guess.

Most API breaking changes should be done now.

I guess this method is from time when I tried to remove TileCache and rely only on JCS Cache but I had to abandon this approach due to performance reasons.

comment:65 Changed 8 months ago by bastiK

In [o33211] - [jmapviewer] class TileAnchor not needed in jmapviewer -> move to JOSM

comment:66 Changed 8 months ago by bastiK

In 11846/josm:

see #7427 - move class TileAnchor from jmapviewer to JOSM

Changed 7 months ago by bastiK

Attachment: reprojection.patch added

comment:67 Changed 7 months ago by bastiK

Attached patch adds reprojection support for TMS, WMS + WMTS.

comment:68 Changed 7 months ago by bastiK

In [o33219] - [jmapviewer] see ​#7427 - remove 'final' + extend toString()

comment:69 Changed 7 months ago by bastiK

Resolution: fixed
Status: assignedclosed

In 11858/josm:

fixed #7427 - Support reprojection (warping) of imagery layer

comment:70 Changed 7 months ago by bastiK

Please test and report any problems!

comment:71 Changed 7 months ago by bastiK

In 11859/josm:

see #7427 - fix test-compile

comment:72 in reply to:  70 Changed 7 months ago by Don-vip

Replying to bastiK:

Please test and report any problems!

Can you please add Javadoc for ImageryLayer.getNativeProjections()? It's now missing for some plugins but I don't know if it's mandatory, optional, null-safe, etc.

comment:73 Changed 7 months ago by Don-vip

In 11860/josm:

see #7427 - checkstyle

comment:74 Changed 7 months ago by Don-vip

15 tests are failing:

 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testParserForMissingStyleIdentifier	0.82 s	1
 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testWIEN	0.69 s	1
 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testGeoportalORTOPL2180	0.73 s	1
 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testGeoportalORTOPL4326	0.7 s	1
 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testParserForDuplicateTags	0.72 s	1
 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testGeoportalTOPOPL	0.67 s	1
 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testTicket12168	0.82 s	1
 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testPseudoMercator	0.63 s	1
 org.openstreetmap.josm.data.imagery.WMTSTileSourceTest.testWALLONIE	0.58 s	1
 org.openstreetmap.josm.gui.NavigatableComponentTest.testZoomToFactor	1.2 s	1
 org.openstreetmap.josm.gui.NavigatableComponentTest.testZoomToFactorCenter	1 s	1
 org.openstreetmap.josm.gui.mappaint.MapCSSRendererTest.testRender[node-shapes]	3.3 s	1
 org.openstreetmap.josm.gui.mappaint.MapCSSRendererTest.testRender[node-text]	0.71 s	1
 org.openstreetmap.josm.gui.mappaint.MapCSSRendererTest.testRender[area-icon]	0.39 s	1
 org.openstreetmap.josm.gui.mappaint.MapCSSRendererTest.testRender[order]	0.57 s	1

comment:75 Changed 7 months ago by Don-vip

In 11862/josm:

see #7427 - sonar - squid:S2259 - Null pointers should not be dereferenced

comment:76 Changed 7 months ago by Don-vip

In 11863/josm:

see #7427 - sonar - squid:S1449 - Locale should be used in String operations

comment:77 Changed 7 months ago by Don-vip

In 11864/josm:

see #7427 - sonar - squid:UselessImportCheck - Useless imports should be removed

comment:78 Changed 7 months ago by Don-vip

In 11865/josm:

see #7427 - checkstyle

comment:79 Changed 7 months ago by Don-vip

In 11866/josm:

see #7427 - PMD

comment:80 Changed 7 months ago by bastiK

In 11867/josm:

see #7427 - fix some unit tests

comment:81 Changed 7 months ago by bastiK

In 11868/josm:

see #7427 - add javadoc for getNativeProjections() + move it one class up

comment:82 Changed 7 months ago by bastiK

In 11877/josm:

see #7427 - WMTS: if no default layer specified, choose first available (fixes tests)

comment:83 Changed 7 months ago by bastiK

In 11882/josm:

see #7427 - minor style change

comment:84 Changed 7 months ago by bastiK

In 11883/josm:

see #7427 - actually use disk cache on scale change and do not force redownload

comment:85 in reply to:  19 Changed 7 months ago by bastiK

Replying to Don-vip:

OK. Did you look at jt-warp? It's licensed under Apache 2.0.

I had another look at this library and basically what they are doing, is to extend the JAI classes for nodata support. This is the main selling point of jai-ext: a magic pixel value or range, indicating there is no data for a region of an image. Not exactly something we need, but the code is still interesting:

  • It shows how to make use of the internal image data structure for maximum efficiency. This gets a bit unwieldy, with several cases and code duplication, so not something one can easily copy & paste.
  • It demonstrates some nice tricks and efficient data layout of the JAI library.

The new custom implementation ImageWarp does what it should, but in terms of runtime it is still pretty slow. Thanks to the caching, it doesn't have to be super efficient. But it may still be noticeably slow, especially for 512x512 tiles (test reports welcome).

The main reason for low efficiency is not pixel lookup and color interpolation, but coordinate conversion from one projection to another. Currently, this is done once for each pixel. An obvious optimization would be to take a grid with 10 px spacing, transform only every 10th pixel and interpolate in between (expected improvement: factor 100). In JAI, they have anticipated this problem, the class javax.media.jai.WarpGrid is precisely what would be needed here.

comment:86 Changed 7 months ago by stoecker

Can you check dropping "epsg4326to3857Supported" handling and using reprojection instead. That should mean that Mercator now looks good for lower zoom levels in case the WMS only supports EPSG:4326 (This is not the case with the workaround).

comment:87 in reply to:  86 Changed 7 months ago by bastiK

Replying to stoecker:

Can you check dropping "epsg4326to3857Supported" handling and using reprojection instead. That should mean that Mercator now looks good for lower zoom levels in case the WMS only supports EPSG:4326 (This is not the case with the workaround).

The 4326to3857 code should be gone already.

comment:88 Changed 7 months ago by stoecker

In 11884/josm:

see #7427 - drop old attribute

comment:89 Changed 7 months ago by Don-vip

In 11891/josm:

see #7427 - sonar - squid:S1226 - Method parameters, caught exceptions and foreach variables should not be reassigned

comment:90 Changed 7 months ago by bastiK

In 11894/josm:

see #7427 - make sure fixed scale is used for alignment + javadoc

comment:91 Changed 7 months ago by bastiK

In 11895/josm:

see #7427 - avoid code duplication (loop over colors)

comment:92 Changed 7 months ago by bastiK

In 11896/josm:

see #7427 - clean up comment

comment:93 Changed 7 months ago by bastiK

In 11897/josm:

see #7427 - optimize warp transformaion

performance problems should be resolved now

comment:94 Changed 7 months ago by Don-vip

2 new findbugs warnings:

ImageWarp.java:61, URF_UNREAD_FIELD
UrF: Unread field: org.openstreetmap.josm.tools.ImageWarp$GridTransform.deletedRows

ImageWarp.java:110, DLS_DEAD_LOCAL_STORE
DLS: Dead store to del in org.openstreetmap.josm.tools.ImageWarp$GridTransform.cleanUp(int)

comment:95 Changed 7 months ago by bastiK

Not sure what to do about it - nothing wrong with the code, as far as I can tell. :)

comment:96 Changed 7 months ago by Don-vip

Problem comes from private static final boolean CONSISTENCY_TEST = false;. Findbugs operate on bytecode, so it does not guess you want to sometimes set it to true for debugging purposes. Is it still needed?

comment:97 Changed 7 months ago by bastiK

Yes, it is fairly easy to misuse this class. The code would still run without error, but performance-wise it would have an adverse effect. The consistency test catches this and also documents the expected behavior.

Basically it is a poor man's unit test and only needs to be enabled for a bit whenever the related code changes. I can also activate the consistency test permanently - it shouldn't make much of a difference.

comment:98 Changed 7 months ago by Don-vip

Why do not enabling this feature in debug or trace mode, rather than through a static boolean? Everyone could enable it without recompiling JOSM, and Findbugs would be happy.

comment:99 Changed 7 months ago by bastiK

In 11915/josm:

see #7427 - run consistency test in debug mode

Changed 7 months ago by Klumbumbus

Attachment: DimmingTiles.png added

comment:100 Changed 7 months ago by Klumbumbus

Resolution: fixed
Status: closedreopened

When dimming a tile layer (here bing) lines appear at the tile borders. They also appear sometimes even without dimming.

(Position: https://www.openstreetmap.org/#map=19/53.03892/14.10883)


comment:101 Changed 7 months ago by bastiK

Are you using Mercator or a different projection?

comment:102 Changed 7 months ago by Klumbumbus

Mercator

comment:103 in reply to:  100 Changed 7 months ago by Klumbumbus

Replying to Klumbumbus:

They also appear sometimes even without dimming.

addition: they are black in this case, i.e. there is a little gap between the tiles

comment:104 Changed 7 months ago by wiktorn

I've noticed the same. This is especially visible with layers with monotone colours.

One of such examples is ISOK hillshade in Poland - mostly greyish.

WMTS: http://mapy.geoportal.gov.pl/wss/service/WMTS/guest/wmts/ISOK_CIEN?REQUEST=GetCapabilities&SERVICE=WMTS{header(User-Agent,Mozilla/5.0 (JOSM)}

comment:105 Changed 7 months ago by bastiK

@Klumbumbus: Thanks for reporting, this in not something I have seen so far. I wasn't able to reproduce. Could you start JOSM in debug mode and check again? There will be a thin red outline, but hopefully the seam is still visible underneath. I'm interested in the size of the boxes, i.e. is it 256x256 px or larger? Please add Tile info dialog from the right click menu if you make another screen shot.

@wiktorn: What is the server projection and what is the display projection?

comment:106 Changed 7 months ago by Klumbumbus

Currently I can't really reproduce this. Lines appear when panning and zooming around when tiles from another zoomlevel are displayed. But in the end when all tiles are loded the lines are gone.

(However when doing the screenshot the lines were permanent.)

comment:107 Changed 7 months ago by wiktorn

In my case: server projection is EPSG:2180 and display projection - Mercator.

And I've just run in the debug mode. The black lines are the same size as the tiles.

If I take Bing layer as a background and use display projection: EPSG:2180, then the lines are also visible (but far less than with ISOK hillshade at Mercator display projection).

comment:108 in reply to:  105 Changed 7 months ago by Klumbumbus

Replying to bastiK:

There will be a thin red outline, but hopefully the seam is still visible underneath.

It seems the red lines hide the other lines

comment:109 in reply to:  106 Changed 7 months ago by bastiK

Replying to Klumbumbus:

Currently I can't really reproduce this. Lines appear when panning and zooming around when tiles from another zoomlevel are displayed. But in the end when all tiles are loded the lines are gone.

Confirmed, I'll have a look.

(However when doing the screenshot the lines were permanent.)

If reproducible, this should be investigated.

comment:110 in reply to:  107 Changed 7 months ago by bastiK

Replying to wiktorn:

In my case: server projection is EPSG:2180 and display projection - Mercator.

And I've just run in the debug mode. The black lines are the same size as the tiles.

If I take Bing layer as a background and use display projection: EPSG:2180, then the lines are also visible (but far less than with ISOK hillshade at Mercator display projection).

When displaying the imagery in the native projection, it should work just as before, i.e. no black lines between tiles.

However, when reprojecting the tiles one by one, I wasn't able to avoid the seams so far and basically considered it an acceptable imperfection. Please note that the pixel positions are still accurate, i.e. there is no "physical gap" that would break a diagonal straight line.

Maybe there is a simple fix to improve this "feature". It can certainly be avoided by requesting pixel data from the 4 adjacent tiles during warp operation. But this seems excessively complicated.

comment:111 Changed 7 months ago by wiktorn

Sorry for the noise then. That was what I thought when I saw them for the first time. And I did not read Kulumbus comment carefully enough to notice that he was displaying tiles with no reprojection.

comment:112 Changed 7 months ago by bastiK

In 11953/josm:

see #7427 - make sure layer is painted only once after zoom change

comment:113 Changed 7 months ago by bastiK

In 11954/josm:

see #7427 - fix seams for reprojected tiles

comment:114 Changed 7 months ago by bastiK

In 11956/josm:

see #7427 - repaint after clear tile cache action

comment:115 Changed 7 months ago by bastiK

In 11958/josm:

see #7427 - clear memory cache on projection change

comment:116 Changed 7 months ago by bastiK

In 11961/josm:

see #7427 - avoid black lines at tile borders (primarily for native tiles)

comment:117 Changed 7 months ago by bastiK

The black lines should be pretty much gone now, you can test again.

comment:118 Changed 7 months ago by Don-vip

It seems to work fine :)

It would be interesting to have a unit test for ReprojectionTile, do you see how to write a meaningful one?

Last edited 7 months ago by Don-vip (previous) (diff)

comment:119 Changed 7 months ago by bastiK

In 11972/josm:

see #7427 - remove obsolete code

comment:120 in reply to:  118 Changed 7 months ago by bastiK

Resolution: fixed
Status: reopenedclosed

Replying to Don-vip:

It would be interesting to have a unit test for ReprojectionTile, do you see how to write a meaningful one?

Difficult for this class, but would be good for some of the other new methods.

comment:121 Changed 7 months ago by stoecker

I'm impressed what already has been done in this ticket. Display of reprojection and also non reprojected tiles is really seemless. Lots of cases with slight distortion at the tile boundary are gone. That's excellent!

Nevertheless there is always a bit to improve. :-) I tried downloading Berlin WMS and while reprojected data is seemless there are small black stripes when you zoom in and out. It seems that the display code for the tiles of up-/downscaled zoom is not as perfect as the current zoom level...

comment:122 in reply to:  121 Changed 7 months ago by bastiK

Replying to stoecker:

Nevertheless there is always a bit to improve. :-) I tried downloading Berlin WMS and while reprojected data is seemless there are small black stripes when you zoom in and out. It seems that the display code for the tiles of up-/downscaled zoom is not as perfect as the current zoom level...

Yes I'm aware of this, the thing is that reprojected tiles simply don't fit together seamlessly when one comes from a different zoom level and is up- or downscaled. This happens to work well for standard tiles, because they are square shaped and the tile size in pixels is a power of 2. All this is no longer true for a reprojected tile - you may have noticed that it is often slightly rotated and the tile boundary may be curved. I do not have a good solution for this right now.

comment:123 Changed 7 months ago by stoecker

If you can't fix it, can't you cheat? A black pixel is very good visible. A pixel with the same colour as its neighbor hardly.

comment:124 Changed 7 months ago by bastiK

If you increase the transparency, you'll notice there are not only gaps but also overlapping parts. So for the cosmetic fix you'd need a pixel-precise mask of the tile in current zoom level, then scale the replacement tile, clip the excess parts and fill in the gaps by extrapolating with the pixel color at the mask outline.

This may be too slow for event dispatch thread, which makes it even more awkward to implement.

Pretty complicated overall, I think, I'll postpone to next month and see if an easier solution emerges.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain bastiK.
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.