Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Bing Maps Styles - AerialWithLabelsOnDemand and RoadOnDemand. #7808

Merged
merged 5 commits into from
May 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
Change Log
==========
### 1.58 - 2019-06-01

##### Additions :tada:
* Added support for new Bing Maps imagery styles RoadOnDemand and AerialWithLabelsOnDemand. The other versions of these, Road and AerialWithLabels, have been deprecated. [#7808] (https://github.com/AnalyticalGraphicsInc/cesium/pull/7808)

### 1.57 - 2019-05-01

Expand Down
3 changes: 2 additions & 1 deletion CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,13 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
* [Omar Shehata](https://github.com/OmarShehata)
* [Matt Petry](https://github.com/MattPetry)
* [Michael Squires](https://github.com/mksquires)
* [NICTA](http://www.nicta.com.au/)
* [NICTA/CSIRO's Data61](https://www.data61.csiro.au/)
* [Kevin Ring](https://github.com/kring)
* [Keith Grochow](https://github.com/kgrochow)
* [Chloe Chen](https://github.com/chloeleichen)
* [Arthur Street](https://github.com/RacingTadpole)
* [Alex Gilleran](https://github.com/AlexGilleran)
* [Emma Krantz](https://github.com/KeyboardSounds)
* [EU Edge](http://euedge.com/)
* [Ákos Maróy](https://github.com/akosmaroy)
* [Raytheon Intelligence and Information Systems](http://www.raytheon.com/)
Expand Down
4 changes: 4 additions & 0 deletions Source/Core/Resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,10 @@ define([
window.URL.revokeObjectURL(generatedBlobResource.url);
}

// If the blob load succeeded but the image decode failed, provide access to the blob on the error object because it may provide useful insight.
// In particular, BingMapsImageryProvider uses this to detect the zero-length "image" that some map styles return when a real tile is not available.
error.blob = generatedBlob;

return when.reject(error);
});
};
Expand Down
56 changes: 41 additions & 15 deletions Source/Scene/BingMapsImageryProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ define([
'../ThirdParty/when',
'./BingMapsStyle',
'./DiscardMissingTileImagePolicy',
'./DiscardEmptyTileImagePolicy',
'./ImageryProvider'
], function(
BingMapsApi,
Expand All @@ -39,6 +40,7 @@ define([
when,
BingMapsStyle,
DiscardMissingTileImagePolicy,
DiscardEmptyTilePolicy,
ImageryProvider) {
'use strict';

Expand All @@ -61,15 +63,16 @@ define([
* for information on the supported cultures.
* @param {Ellipsoid} [options.ellipsoid] The ellipsoid. If not specified, the WGS84 ellipsoid is used.
* @param {TileDiscardPolicy} [options.tileDiscardPolicy] The policy that determines if a tile
* is invalid and should be discarded. If this value is not specified, a default
* {@link DiscardMissingTileImagePolicy} is used which requests
* tile 0,0 at the maximum tile level and checks pixels (0,0), (120,140), (130,160),
* (200,50), and (200,200). If all of these pixels are transparent, the discard check is
* disabled and no tiles are discarded. If any of them have a non-transparent color, any
* tile that has the same values in these pixel locations is discarded. The end result of
* these defaults should be correct tile discarding for a standard Bing Maps server. To ensure
* that no tiles are discarded, construct and pass a {@link NeverTileDiscardPolicy} for this
* parameter.
* is invalid and should be discarded. The default value will depend on the map style. If
* `BingMapsStyle.AERIAL_WITH_LABELS_ON_DEMAND` or `BingMapsStyle.ROADS_ON_DEMAND` is used, then a
* {@link DiscardEmptyTileImagePolicy} will be used to handle the Bing Maps API sending no content instead of
* a missing tile image, a behaviour specific to that imagery set. In all over cases, a default
* {@link DiscardMissingTileImagePolicy} is used which requests tile 0,0 at the maximum tile level and checks
* pixels (0,0), (120,140), (130,160), (200,50), and (200,200). If all of these pixels are transparent, the
* discard check is disabled and no tiles are discarded. If any of them have a non-transparent color, any
* tile that has the same values in these pixel locations is discarded. The end result of these defaults
* should be correct tile discarding for a standard Bing Maps server. To ensure that no tiles are discarded,
* construct and pass a {@link NeverTileDiscardPolicy} for this parameter.
*
* @see ArcGisMapServerImageryProvider
* @see GoogleEarthEnterpriseMapsProvider
Expand Down Expand Up @@ -171,11 +174,18 @@ define([

// Install the default tile discard policy if none has been supplied.
if (!defined(that._tileDiscardPolicy)) {
that._tileDiscardPolicy = new DiscardMissingTileImagePolicy({
missingImageUrl : buildImageResource(that, 0, 0, that._maximumLevel).url,
pixelsToCheck : [new Cartesian2(0, 0), new Cartesian2(120, 140), new Cartesian2(130, 160), new Cartesian2(200, 50), new Cartesian2(200, 200)],
disableCheckIfAllPixelsAreTransparent : true
});
// Our default depends on which map style we're using.
if (that._mapStyle === BingMapsStyle.AERIAL_WITH_LABELS_ON_DEMAND
|| that._mapStyle === BingMapsStyle.ROAD_ON_DEMAND) {
// this map style uses a different API, which returns a tile with no data instead of a placeholder image
that._tileDiscardPolicy = new DiscardEmptyTilePolicy();
} else {
that._tileDiscardPolicy = new DiscardMissingTileImagePolicy({
missingImageUrl : buildImageResource(that, 0, 0, that._maximumLevel).url,
pixelsToCheck : [new Cartesian2(0, 0), new Cartesian2(120, 140), new Cartesian2(130, 160), new Cartesian2(200, 50), new Cartesian2(200, 200)],
disableCheckIfAllPixelsAreTransparent : true
});
}
}

var attributionList = that._attributionList = resource.imageryProviders;
Expand Down Expand Up @@ -533,7 +543,23 @@ define([
}
//>>includeEnd('debug');

return ImageryProvider.loadImage(this, buildImageResource(this, x, y, level, request));
var promise = ImageryProvider.loadImage(this, buildImageResource(this, x, y, level, request));

if (defined(promise)) {
return promise.otherwise(function(error) {

// One possible cause of an error here is that the image we tried to load was empty. This isn't actually
// a problem. In some imagery sets (eg. `BingMapsStyle.AERIAL_WITH_LABELS_ON_DEMAND`), an empty image is
// returned rather than a blank "This Image is Missing" placeholder image. In this case, we supress the
// error.
if (defined(error.blob) && error.blob.size === 0) {
return DiscardEmptyTilePolicy.EMPTY_IMAGE;
}
return when.reject(error);
});
}

return undefined;
};

/**
Expand Down
20 changes: 20 additions & 0 deletions Source/Scene/BingMapsStyle.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,37 @@ define([
*
* @type {String}
* @constant
* @deprecated See https://github.com/AnalyticalGraphicsInc/cesium/issues/7128.
* Use `BingMapsStyle.AERIAL_WITH_LABELS_ON_DEMAND` instead
*/
AERIAL_WITH_LABELS : 'AerialWithLabels',

/**
* Aerial imagery with a road overlay.
*
* @type {String}
* @constant
*/
AERIAL_WITH_LABELS_ON_DEMAND : 'AerialWithLabelsOnDemand',

/**
* Roads without additional imagery.
*
* @type {String}
* @constant
* @deprecated See https://github.com/AnalyticalGraphicsInc/cesium/issues/7128.
* Use `BingMapsStyle.ROAD_ON_DEMAND` instead
*/
ROAD : 'Road',

/**
* Roads without additional imagery.
*
* @type {String}
* @constant
*/
ROAD_ON_DEMAND : 'RoadOnDemand',

/**
* A dark version of the road maps.
*
Expand Down
44 changes: 44 additions & 0 deletions Source/Scene/DiscardEmptyTileImagePolicy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
define([
'../Core/defined',
'../Core/defaultValue'
], function(
defined,
defaultValue) {
'use strict';

/**
* A policy for discarding tile images that contain no data (and so aren't actually images).
*
* @alias DiscardEmptyTileImagePolicy
* @constructor
*
* @see DiscardMissingTileImagePolicy
*/
function DiscardEmptyTileImagePolicy(options) {
}

/**
* Determines if the discard policy is ready to process images.
* @returns {Boolean} True if the discard policy is ready to process images; otherwise, false.
*/
DiscardEmptyTileImagePolicy.prototype.isReady = function() {
return true;
};

/**
* Given a tile image, decide whether to discard that image.
*
* @param {Image} image An image to test.
* @returns {Boolean} True if the image should be discarded; otherwise, false.
*/
DiscardEmptyTileImagePolicy.prototype.shouldDiscardImage = function(image) {
return DiscardEmptyTileImagePolicy.EMPTY_IMAGE === image;
};

/**
* Default value for representing an empty image.
*/
DiscardEmptyTileImagePolicy.EMPTY_IMAGE = {};

return DiscardEmptyTileImagePolicy;
});
21 changes: 21 additions & 0 deletions Specs/Core/ResourceSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1310,6 +1310,27 @@ defineSuite([
expect(error).toBeInstanceOf(RequestErrorEvent);
});
});

it('rejects the promise with extra error information when image errors and options.preferBlob is true', function() {
if (!supportsImageBitmapOptions) {
return;
}

// Force the fetching of a bad blob that is not an image to trigger the error
spyOn(Resource.prototype, 'fetch').and.returnValue(when.resolve(new Blob([new Uint8Array([])], { type: 'text/plain' })));

return Resource.fetchImage({
url: 'http://example.invalid/testuri.png',
preferImageBitmap: true,
preferBlob: true
})
.then(function() {
fail('expected promise to reject');
})
.otherwise(function(error) {
expect(error.blob).toBeInstanceOf(Blob);
});
});
});

describe('fetchImage without ImageBitmap', function() {
Expand Down
45 changes: 43 additions & 2 deletions Specs/Scene/BingMapsImageryProviderSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ defineSuite([
'Scene/ImageryProvider',
'Scene/ImageryState',
'Specs/pollToPromise',
'ThirdParty/Uri'
'ThirdParty/Uri',
'ThirdParty/when',
'Scene/DiscardEmptyTileImagePolicy'
], function(
BingMapsImageryProvider,
appendForwardSlash,
Expand All @@ -31,7 +33,9 @@ defineSuite([
ImageryProvider,
ImageryState,
pollToPromise,
Uri) {
Uri,
when,
DiscardEmptyTileImagePolicy) {
'use strict';

var supportsImageBitmapOptions;
Expand Down Expand Up @@ -498,4 +502,41 @@ defineSuite([
});
});
});

it('correctly handles empty tiles', function() {
var url = 'http://foo.bar.invalid';
var mapStyle = BingMapsStyle.ROAD_ON_DEMAND;

installFakeMetadataRequest(url, mapStyle);

var provider = new BingMapsImageryProvider({
url : url,
mapStyle : mapStyle
});

var layer = new ImageryLayer(provider);

// Fake ImageryProvider.loadImage's expected output in the case of an empty tile
var e = new Error();
e.blob = {size: 0};
var errorPromise = when.reject(e);

spyOn(ImageryProvider, 'loadImage').and.returnValue(errorPromise);

return pollToPromise(function() {
return provider.ready;
}).then(function() {
var imagery = new Imagery(layer, 0, 0, 0);
imagery.addReference();
layer._requestImagery(imagery);
RequestScheduler.update();

return pollToPromise(function() {
return imagery.state === ImageryState.RECEIVED;
}).then(function() {
expect(imagery.image).toBe(DiscardEmptyTileImagePolicy.EMPTY_IMAGE);
imagery.releaseReference();
});
});
});
});
54 changes: 54 additions & 0 deletions Specs/Scene/DiscardEmptyTileImagePolicySpec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
defineSuite([
'Scene/DiscardEmptyTileImagePolicy',
'Core/Resource',
'Specs/pollToPromise',
'ThirdParty/when'
], function(
DiscardEmptyTileImagePolicy,
Resource,
pollToPromise,
when) {
'use strict';

afterEach(function() {
Resource._Implementations.createImage = Resource._DefaultImplementations.createImage;
Resource._Implementations.loadWithXhr = Resource._DefaultImplementations.loadWithXhr;
});

describe('shouldDiscardImage', function() {
fit('does not discard a non-empty image', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fit snuck into the committed code here, which disables all other tests. (this sort of thing is why fit should just not exist in Jasmine, in my opinion)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fit and fdescribe are really helpful if you just need to debug one new test. However, in some of our other repos we added a custom eslint rule to catch it so it would break CI if you committed it. Maybe we should add that to this repo too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec filtering can run single tests or suites without needing to edit the source, e.g.:

http://localhost:8080/Specs/SpecRunner.html?spec=Core%2FappendForwardSlash%20Appends%20to%20a%20url

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work if you need to run it from the command line to reproduce an error that only happens in travis

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our custom web client is honestly crazy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't our eslint configuraton flag fit/fdescribe? It does everywhere else outside of Cesium.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm pretty sure it's just in the eslintrc.json for specific projects

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Play. We should definitely fix that then, I'll open a PR soon-ish

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #7809

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How embarrassing, good catch. 😳

var promises = [];
promises.push(Resource.fetchImage('Data/Images/Green4x4.png'));

var policy = new DiscardEmptyTileImagePolicy();

promises.push(pollToPromise(function() {
return policy.isReady();
}));

return when.all(promises, function(results) {
var greenImage = results[0];

expect(policy.shouldDiscardImage(greenImage)).toEqual(false);
});
});

fit('discards an empty image', function() {
var promises = [];
promises.push(when.resolve(DiscardEmptyTileImagePolicy.EMPTY_IMAGE));

var policy = new DiscardEmptyTileImagePolicy();

promises.push(pollToPromise(function() {
return policy.isReady();
}));

return when.all(promises, function(results) {
var emptyImage = results[0];

expect(policy.shouldDiscardImage(emptyImage)).toEqual(true);
});
});

});
});