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

Tentative fix of combined compilation. #82

Merged
merged 2 commits into from
Sep 30, 2014
Merged

Tentative fix of combined compilation. #82

merged 2 commits into from
Sep 30, 2014

Conversation

gberaudo
Copy link
Member

var billboards = new Cesium.BillboardCollection();
var reallyCreateBillboard = function() {
var center = geometry.getCoordinates();
var position = olcs.core.ol4326CoordinateToCesiumCartesian(center);
goog.isDefAndNotNull(image);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be nicer to either goog.asserts.assert(goog.isDefAndNotNull(image));, or to wrap the block below in an if (goog.isDefAndNotNull(image)) {} block?

Copy link
Member

Choose a reason for hiding this comment

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

Agree. I don't understand what calling goog.isDefAndNotNull(image) does here.

@ahocevar
Copy link
Member

One minor comment, but looks good otherwise.

@@ -514,18 +514,18 @@ goog.require('olcs.core.OLImageryProvider');
geometry = olGeometryCloneTo4326(geometry, projection);

var imageStyle = style.getImage();
var image = imageStyle.getImage();
var image = imageStyle.getImage(1); // get normal density
Copy link
Member

Choose a reason for hiding this comment

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

Without openlayers/openlayers#2754 this change should make the compilation fail when building ol3cesium.js (make dist). So openlayers/openlayers#2754 is related to this PR.

@@ -111,7 +111,7 @@ Cesium.BillboardCollection = function() {};

/**
* @typedef {{
* image: string,
* image: (string|HTMLCanvasElement|HTMLVideoElement|Image),
Copy link
Member

Choose a reason for hiding this comment

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

Do we know that image can actually reference an HTMLCanvasElement or an HTMLVideoElement here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Cesium docs state it can be an image, canvas (they define the type as Canvas, which I'm assuming is the same as HTMLCanvasElement?), URL, or callback. An HTMLVideoElement would likely fail.

@elemoine
Copy link
Member

Please update the ol3 sub-module and merge.

gberaudo added a commit that referenced this pull request Sep 30, 2014
@gberaudo gberaudo merged commit 6a17951 into openlayers:master Sep 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants