Tentative fix for issue 416: TileMapServiceImageryProvider extent #1010
Conversation
| @@ -0,0 +1,76 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
Instead of adding a new Sandcastle app, you should add unit tests for this. Sandcastle apps are meant to demonstrate things to end-users, not to test Cesium for correct functionality.
There was a problem hiding this comment.
Indeed it is my first bug fix, I find using Sandcastle for testing the most simple way. Could you point me to a unit test sample ?
There was a problem hiding this comment.
Tests are found in the Specs directory:
https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Specs
The tests for ImageryLayer are found here:
https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Specs/Scene/ImageryLayerSpec.js
There was a problem hiding this comment.
I read the 2 unit test you wrote for fixing the imagery stretching issue ( #970 ). If I understand correctly the imagery of a base layer is always stretched to cover the whole ellipsoid, while for a non-base layer this is not the case and the real width and height are preserved.
I noticed you created a specific imagery source (Data/TMS/SmallArea). Indeed there is only the xml file used for defining the imagery parameters. Referring to the base layer case, it is not clear to me why the west terrain tile is made up by 4 imageries and the other tile by 2 imageries only. It should be dependent on the TilingScheme of the TileMapServiceImageryProvider, shouldn't it ? In general given a simple imagery source like the one you defined, how can I foresee how many imagery pieces are included inside a specific terrain tile (not necessary a zero level tile) ?
There was a problem hiding this comment.
Yeah, it's a little confusing. Basically, the SmallArea/tilemapresource.xml file used in the test specifies that the first level is level 11, it uses the Web Mercator tiling scheme, and that the overall extent of the imagery is as specified. If you do the math on that (you can get WebMercatorTilingScheme to do it for you) that means there are 4 tiles at the first level, level 11.
Meanwhile, our terrain, which is the default EllipsoidTerrainProvider, has two tiles at level 0, one covering the western hemisphere and one the eastern. And our test is computing which imagery tiles overlap these two tiles.
All four imagery tiles are in the western hemisphere, so they all overlap the western hemisphere terrain tile. That's why there are four for tiles[0]. None of them overlap the eastern hemisphere terrain tile, though, so why is tiles[1].imagery.length expected to be 2? That's because the imagery layer is a base layer, so the two tiles eastern-most imagery tiles are stretched all the way to the eastern hemisphere of the globe.
I'm not completely sure if I've answered your question, so let me know if that helps.
There was a problem hiding this comment.
Thank you for your explanation. I think to have understood. Moreover, if I got it correctly, both terrain and imagery tiles at each level become 4 times the tiles of the previous level, don't they ?
I would need some hint on how to design the unit test for my fix. Should I create a virtual imagery source (as your SmallArea) and define the extent attribute passed to the TileMapServiceImageryProvider in a way that I know what are the textureCoordinateExtent of each TileImagery that is included in a terrain tile ? For instance I could set the extent attribute to a value that for tiles[0] the first imagery has a bounding box [0.2, 0.5, 0.5, 0.8] the second imagery has a bbox [0.2, 0.2, 0.5, 0.5] and so on; no intersection with tiles[1] and then other precomputed bbox for children terrain tiles of tiles[0]. Would it be an effective way to write the unit test ? How much error tollerance should I use in comparing computed values with expected ones ? That is, instead of a bbox [0.2, 0.5, 0.5, 08] I could get [0.2001, 0.2, 0.4999, 0.5] .
There was a problem hiding this comment.
Sorry for the late response on this!
Yes, in general, each tile has four child tiles. All four children don't necessarily exist, though.
Your proposed test sounds reasonable to me. I think if you compute what the texture coordinate extent should be, you can expect the answer Cesium comes up with to be very close to that. Maybe within 1e-14 or so, given that the numbers are all less than 1.0.
|
Thanks for the pull request, @mcecchetti! Do we already have a Contributor's License Agreement (CLA) from you? Your fix doesn't work in the case that the layer with a limited extent is the base layer. To see what I mean, uncomment |
|
On Mon, 05 Aug 2013 23:57:11 +0200, Kevin Ring [email protected]
Not yet, should I print the CLA and sign it ?
Ah, I will give a glance. Using Opera's revolutionary e-mail client: http://www.opera.com/mail/ |
See here: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/CONTRIBUTING.md You can fill it out electronically and email it to [email protected]. |
|
I mailed the CLA |
We got it. Thanks! |
|
Is this ready to merge? It has been open for a while. |
|
I am sorry, but lately I have been busy, I have not yet implemented the related test suite code |
|
What is the plan here? This has been opened for 8 months. |
|
I believe this was fixed in #2181. Please re-open if this is not the case. |
The fix I implemented intersect
the extent of each tile (of the imagery service provider tiling scheme) which
contains some part of the imagery with the imagery extent.
However I noticed that the extent of each cached imagery (an imagery for each
tile) has to be the extent of the tile and not the extent of the intersection.
I committed also a Sandcastle example (Test Imagery Layer) that I utilized for
testing.