-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Support imagery in alternate map projections #7438
Conversation
Thanks for the pull request @likangning93!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Source/Scene/ImageryState.js
Outdated
@@ -15,7 +15,8 @@ define([ | |||
READY : 4, | |||
FAILED : 5, | |||
INVALID : 6, | |||
PLACEHOLDER : 7 | |||
PLACEHOLDER : 7, | |||
EMPTY : 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
@likangning93 would automatically handling imagery servers that support different projections be possible/easy once this is done? See #7435 which shows that ArcGisMapServerImageryProvider should be able to detect other projections and act accordingly, right? (Not for projections MVP, but something we should probably support eventually) |
I think so? It might be tricky if |
@bagnell this is ready for a look. |
@bagnell bump |
projectedRectangle : new Cesium.Rectangle(-4194304, -4194304, 4194304, 4194304), | ||
numberOfLevelZeroTilesX : 2, | ||
numberOfLevelZeroTilesY : 2, | ||
imageResolution : new Cesium.Cartesian2(512, 512) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imageResolution
isn't an option.
*/ | ||
ProjectedImageryTilingScheme.prototype.getProjectedTilesForNativeTile = function(x, y, level) { | ||
var tileRectangle = this.tileXYToRectangle(x, y, level, new Rectangle()); | ||
var tileProjectedRectangle = Rectangle.approximateProjectedExtents(tileRectangle, this._projection, new Rectangle()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use scratch variables.
Source/Scene/Imagery.js
Outdated
|
||
var tilingScheme = this.imageryLayer.imageryProvider.tilingScheme; | ||
var singleSource = !(tilingScheme instanceof ProjectedImageryTilingScheme); | ||
var imageryLayer = this.imageryLayer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to the top of the function and replace the other this.imageryLayer
s.
Source/Scene/Imagery.js
Outdated
var index = i * 2; | ||
var x = projectedIndices[index]; | ||
var y = projectedIndices[index + 1]; | ||
this.projectedRectangles[i] = tilingScheme.getProjectedTileProjectedRectangle(x, y, level); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass this.projectedRectangles[i]
as the result parameter. This way it doesn't always create new Rectangles.
Source/Scene/ImageryLayer.js
Outdated
} | ||
|
||
// Mark discarded imagery tiles invalid. Parent imagery will be used instead. | ||
if (discardPolicy.shouldDiscardImage(projectedImages[0])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this have a mix of images that should be discarded and some that should not? If so, you should discard the ones that need to be and, if all are discarded, set the state to invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discard the ones that need to be
If only some of the images are discarded do we need to fill in the missing areas using the parent image? Is it too heavy-handed to mark the entire tile as invalid if one of its images is discarded?
If we're filling in, maybe we could grab the parent's reprojected image and copy over select areas. We can use the computed texcoords like a mask but read out of the parent texture using the fragment coordinates and another uniform or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grab the parent's reprojected image
There might be complications with this too, if the parent isn't available would we want to go for the nearest ancestor instead? If so, would we want to regenerate the tile when the parent becomes available?
-___-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are good questions. Maybe open an issue? What do you think @lilleyse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a discussion offline, we'll just discard the entire "target" tile for now when any of the source images are invalidated. I'll document the behavior.
Source/Scene/ImageryLayer.js
Outdated
when, | ||
Imagery, | ||
ImagerySplitDirection, | ||
ImageryState, | ||
TileImagery) { | ||
'use strict'; | ||
|
||
var ARBITRARY_PROJECTION_VERTICES_WIDTH = 128; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be configurable?
Source/Scene/Imagery.js
Outdated
} | ||
|
||
if (projectedTilesLength === 0) { | ||
this.state = ImageryState.EMPTY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be marked as invalid instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if imagery was requested past the zoom level of the imagery? It should render the ancestor there. Is there a way to tell the difference between this and the case you posted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, this happens when users don't set a max level and the terrain tile might need an INVALID
imagery
to upsample correctly. Just looping the state machine when the imagery isn't actually "ready" might cause problems, so I'll replace with INVALID
.
|
||
void main() | ||
{ | ||
bool inBounds = 0.0 < v_textureCoordinates.x && v_textureCoordinates.x < 1.0 && 0.0 < v_textureCoordinates.y && v_textureCoordinates.y < 1.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this include 0.0
and 1.0
?
…s should be discarded
@bagnell this is up-to-date! |
It might be better to hold off on merging this until we get the main projection branch into master (since these PRs are related but build on each other as separate features, it would be easier to review |
@likangning93 what's the plan for this PR? |
It wouldn't be ideal, but if nobody has time to review this I can make my peace with closing this unmerged. |
Spoke with @mramato offline, and we're going to leave these open for now and will hopefully be able to finish them up soon |
At this point the branches are out of date and it'd be a lot of effort to get this PR ready. I'll leave the branch around if we want to look at this again in the future. |
Take 2. See discussion in attempt 1 here.
This PR adds support for imagery in alternate projections, for example, the kind of TMS you would get from throwing a geotiff of LIMA through GDAL2Tiles using the
raster
profile. There's also some of this stuff hosted online, like in the new Sandcastle:Reprojection is once again accomplished with a fixed-resolution vertex warp grid that is much less sophisticated than what OpenLayers advertises and might need further discussion.
However, instead of projecting each image into a geographic tile that overlaps its neighbors, we're generating non-overlapping geographic tiles that may draw from 0 or more projected imagery tiles.
This seems to have gotten around most of the craziness in the first attempt.