Address Mapbox Tiling

[robmilton]

When loading maps from Mapbox (and potentially other Mercator layer sources) a problem existed with tiles containing little or no detail (such as over water) appearing blank or empty.

Description of the Change

Discovered that these tile images report various image types which which need to be handled as "BufferedImage.TYPE_INT_ARGB" for the layer to display as intended.

Modified handling of image type in BasicMercatorTiledImageLayer class as follows:
Old Code

                int type = image.getType();
                if (type == BufferedImage.TYPE_CUSTOM)
                {
                    type = BufferedImage.TYPE_INT_RGB;
                }
                else if (type == BufferedImage.TYPE_BYTE_INDEXED)
                {
                    type = BufferedImage.TYPE_INT_ARGB;
                }

New Code

               int type = image.getType();
               if (type != BufferedImage.TYPE_INT_RGB)
               {
                   type = BufferedImage.TYPE_INT_ARGB;
               }

Why Should This Be In Core?

Allows users to include Mapbox layers in WorldWind and potentially any Mercator layer.

Benefits

-- See "Why Should This Be In Core"--

Potential Drawbacks

No real drawback. However, this issue was challenging to track down since using the "wrong" image type did not produce any error or warning messages. If other image types produce display problems in the future it may be difficult to track down the reason as well. Unfortunately I don't know of any good way to provide a warning message in these situations.

Applicable Issues

#53

EDIT
Leaving details about "level of detail calculation" here (below) for future reference. However, the pull request has been modified so that the changes to the "needToSplit" function in the TiledImageLayer class have been reverted to the WWJ v2.1.0 version. Also removed the override of this function from the BasicMercatorTiledImageLayerClass.

Level of Detail Calculation

Issue Description

@Sufaev pointed out that the "needToSplit" calculation in the TiledImageLayer class

is based on camera distance to surface and horizontal field of view, but it does not provide stable pixel density of tiles comparing to monitor pixel density (as it usually appears on 2d maps).

Description of the Change

Moved "needToSplit" function from BasicMercatorTiledImageLayer class to TiledImageLayer class (replacing the same function in the TiledImageLayer class).
Old Code in TiledImageLayer class

    protected boolean needToSplit(DrawContext dc, Sector sector, Level level)
    {
        // Compute the height in meters of a texel from the specified level. Take care to convert from the radians to
        // meters by multiplying by the globe's radius, not the length of a Cartesian point. Using the length of a
        // Cartesian point is incorrect when the globe is flat.
        double texelSizeRadians = level.getTexelSize();
        double texelSizeMeters = dc.getGlobe().getRadius() * texelSizeRadians;

        // Compute the level of detail scale and the field of view scale. These scales are multiplied by the eye
        // distance to derive a scaled distance that is then compared to the texel size. The level of detail scale is
        // specified as a power of 10. For example, a detail factor of 3 means split when the cell size becomes more
        // than one thousandth of the eye distance. The field of view scale is specified as a ratio between the current
        // field of view and a the default field of view. In a perspective projection, decreasing the field of view by
        // 50% has the same effect on object size as decreasing the distance between the eye and the object by 50%.
        // The detail hint is reduced for tiles above 75 degrees north and below 75 degrees south.
        double s = this.getDetailFactor();
        if (sector.getMinLatitude().degrees >= 75 || sector.getMaxLatitude().degrees <= -75)
            s *= 0.9;
        double detailScale = Math.pow(10, -s);
        double fieldOfViewScale = dc.getView().getFieldOfView().tanHalfAngle() / Angle.fromDegrees(45).tanHalfAngle();
        fieldOfViewScale = WWMath.clamp(fieldOfViewScale, 0, 1);

        // Compute the distance between the eye point and the sector in meters, and compute a fraction of that distance
        // by multiplying the actual distance by the level of detail scale and the field of view scale.
        double eyeDistanceMeters = sector.distanceTo(dc, dc.getView().getEyePoint());
        double scaledEyeDistanceMeters = eyeDistanceMeters * detailScale * fieldOfViewScale;

        // Split when the texel size in meters becomes greater than the specified fraction of the eye distance, also in
        // meters. Another way to say it is, use the current tile if its texel size is less than the specified fraction
        // of the eye distance.
        //
        // NOTE: It's tempting to instead compare a screen pixel size to the texel size, but that calculation is
        // window-size dependent and results in selecting an excessive number of tiles when the window is large.
        return texelSizeMeters > scaledEyeDistanceMeters;
    }

New Code in TiledImageLayer class

protected boolean needToSplit(DrawContext dc, Sector sector, Level level)
    {
        double texelSize = level.getTexelSize() * dc.getGlobe().getRadius();
        double pixelSize = dc.getView().computePixelSizeAtDistance(sector.distanceTo(dc, dc.getView().getEyePoint()));
        return texelSize > pixelSize * this.getDetailFactor();
    }

Potential Drawbacks

The "expected" behavior may be subjective. There is also a note in the existing "needToSplit" function in the TiledImageLayer class that reads:
NOTE: It's tempting to instead compare a screen pixel size to the texel size, but that calculation is window-size dependent and results in selecting an excessive number of tiles when the window is large.

The "level of detail calculation" discussion can continue as a separate issue and tied to #60

EDIT 2
Modified title removing reference to "level of detail calculations". Also updated description of the issue and modified the new "needToSplit" code in the "Description of the Change" section to reflect the final version of the code in the pull request.

[wcmatthysen]

@robmilton, @Sufaev, can we merge this into develop?

[robmilton]

I think the only question has to do with the approach used in the level of detail calculation (in the "needToSplit" function). @Sufaev points out that the Web and Android versions of WW use the approach that was pushed in this branch. He also feels that this approach provides the desired tiling behavior.

I'm not entirely sure that I agree with the new behavior (maybe only because I'm used to the current behavior in WWJ). However, I don't have a strong opinion about it.

So I'd say if the majority feel like this "new" approach (at least to WWJ) is the way to go then the pull request is ready to merge.

[EMaksymenko]

If we apply new needToSplit approach, then comments to detailHint variables and related code should be reviewed. New approach use detailHint value in opposite way. Less value provide more detailed tiles. It may cause undesired behavior for some projects which use detailHint variable control. But MercatorLayer already has this behaviour and this provides different behaviour for mercator and non-mercator layers already.

Also it will be good to switch camera calculations to vertical FOV like in Web and Android togather with new detail control, but it will be more serious change than detailHint behaviour change.

I think we should review detailHint comments and vertical FOV to merge this PR or simply revert MercatorLayer neetToSplit to have same behavior. It was my fault that I change it in MercatorLayer before. I thought it utilize detailHint the same way.

May be lets merge only ImageBuffer.type and revert needToSplit from Mercator in this PR to make consistant behaviour of all layers and then change needToSplit to new approach with vertical FOV in separate PR?

[gbburkhardt]

When copying from the input BufferedImage to the output BufferedImage 'trans', only the RGB bands are copied over with

trans.setRGB(x, y, image.getRGB(x, iy));

So why isn't the output BufferedImage 'trans' always TYPE_INT_RGB? It seems to me that one could skip the switch statement entirely, and unconditionally create 'trans' as TYPE_INT_RGB. The code is assuming that 'image.getRGB()' yields valid data regardless of the input type.

[gbburkhardt]

Could someone please add a comment explaining the computation in TiledImageLayer.needToSplit()? It could also use a general comment about what the method is for. Everyone will forget the details in a few months.

[EMaksymenko]

RGB type will provide opaque images in case of overlays. More universal way to use ARGB.

[wcmatthysen]

@robmilton, @Sufaev, perhaps a better approach would be to leave the need-to-split logic out of this pull-request for now and only fix the image-type bug. We can then address the need-to-split logic at a later stage when we are ready (we can keep issue #60 open for this). I think it is a bit hectic to change this logic in TiledImageLayer and subsequently change the behavior for all subclasses especially as we are trying to work towards a stable 2.2.0 release. We can perhaps add this to a 2.3.0 roadmap.

You can add commits to this pull-request by committing the changes to this fixMapboxTilingProblems branch (just remember to push to your remote fork).

[robmilton]

Ok, I just pushed changes that revert the "needToSplit" function in the TiledImageLayer class to what was in WWJ v2.1.0 and removed the override of this function from the BasicMercatorTiledImageLayer class.

The only change in this pull request is the modification to how image types are handled in the BasicMercatorTiledImageLayer class. There is also the addition of the Mapbox layer and the reference to it in the worldwind.layers.xml file.

[EMaksymenko]

@wcmatthysen I totally agree with you. Lets change needToSplit together with vertical FOV in next release.

What about using ARGB type always without any switch logic? Or use it in any cases when type is not RGB to save memory on images which have guaranteed opaque source?

                int type = image.getType();
                if (type != BufferedImage.TYPE_INT_RGB)
                {
                    type = BufferedImage.TYPE_INT_ARGB;
                }

I am afraid that not all TYPE_BYTE_INDEXED or TYPE_BYTE_BINARY images are opaque. Better to use alpha channel to be sure.

[robmilton]

If we're going to take that approach then no "if" statement is needed at all. It should simply be
int type = BufferedImage.TYPE_INT_ARGB;

As I mentioned in an earlier post this is exactly what we do with our extended class. However, this class is only used for Mercator layers. I have not tested this approach with various types of layers.

[EMaksymenko]

TYPE_INT_ARGB theoretically consume more memory then TYPE_INT_RGB when we are sure that source tile is opaque and do not need alpha channel.

[robmilton]

Ah, I see...

You guys let me know if you want me to change to the "if" statement you posted and push that change.

[gbburkhardt]

BufferedImage.getRGB() returns a TYPE_INT_ARGB integer pixel, so setting the new image 'trans' to TYPE_INT_ARGB should be safe, even if the incoming image is TYPE_INT_RGB. But I think that the outgoing image 'trans' should be either TYPE_INT_ARGB or TYPE_INT_RGB.

                    for (int x = 0; x < image.getWidth(); x++)
                    {
                        trans.setRGB(x, y, image.getRGB(x, iy));
                    }
...
                    return trans;

I see that in Java 8, DirectColorModel is used when creating BufferedImage with either TYPE_INT_RGB or TYPE_INT_RGBA, and the alpha value returned with 'getRGB' is computed with

    final public int getAlpha(int pixel) { 
        if (!supportsAlpha) return 255;

so I'm happy with @robmilton's suggestion to always have 'trans' be unconditionally created with TYPE_INT_RGBA.

[EMaksymenko]

You are right, but RGB produce opaque background for overlays like GoogleHybrid, GoogleTraffic and any other map sources which have transparent background and few pixels inside like roads. So RGBA is more universal.

[robmilton]

So where do we stand with this? How would you like to proceed?

[gbburkhardt]

I think you have 3 votes for using ARGB unless the incoming type is RGB as described here.
@wcmatthysen ??

[wcmatthysen]

@gbburkhardt, I agree with always using ARGB, unless the incoming type is RGB. @Sufaev, will you guys add that as a commit to this branch. The current logic for that is:

int type = image.getType();
switch (type) {
    case BufferedImage.TYPE_CUSTOM:
        type = BufferedImage.TYPE_INT_RGB;
        break;
    case BufferedImage.TYPE_BYTE_INDEXED:
        type = BufferedImage.TYPE_INT_ARGB;
        break;
    case BufferedImage.TYPE_BYTE_BINARY:
        type = BufferedImage.TYPE_INT_RGB;
        break;
    default:
        // leave value returned from image.getType()
        break;
}

[EMaksymenko]

It is @robmilton branch, he could make a change. Lets do this change and release PR finally :)
This is the most discussing issue of the year :)

[robmilton]

Ok, I believe it's ready to go.