Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Terrain/imagery cleanup #563

Merged
merged 3 commits into from

3 participants

@kring
Collaborator

Some minor cleanup and tweaks to the terrain/imagery system, from the oneTexturePerLayer branch.

The most significant change here is to use the Context cache to store objects needed for imagery reprojection rather than holding a separate copy per layer.

@pjcozzi pjcozzi commented on the diff
Source/Scene/ImageryLayer.js
@@ -87,6 +87,21 @@ define([
* imagery tile for which the contrast is required, and it is expected to return
* the contrast value to use for the tile. The function is executed for every
* frame and for every tile, so it must be fast.
+ * @param {Number|Function} [description.hue=0.0] The hue of this layer. 0.0 uses the unmodified imagery color.
@pjcozzi Collaborator
pjcozzi added a note

Good catch. @bagnell and I forgot to add these. Funny that they made it into the tutorial though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on the diff
Source/Scene/ImageryLayer.js
((7 lines not shown))
var maximumSupportedAnisotropy = context.getMaximumTextureFilterAnisotropy();
- this._mipmapSampler = context.createSampler({
+ mipmapSampler = context.cache.imageryLayer_mipmapSampler = context.createSampler({
@pjcozzi Collaborator
pjcozzi added a note

Why cache these at all? This is a light object. See context.prototype.createSampler If anything, createSampler could return cached samplers down the road when WebGL has separate, potentially immutable, sampler and texture objects like Direct3D and OpenGL.

@kring Collaborator
kring added a note

If I didn't cache them, I'd still be creating a bunch of unnecessary objects. And the fact that they're lightweight is an implementation detail of Context, no?

@pjcozzi Collaborator
pjcozzi added a note

To me this is the same as caching new Extent(0.0, 0.0, 0.0, 0.0). Let's disagree and commit. Onward. :+1:

If I change createSampler later, I'll update this.

@mramato Collaborator
mramato added a note

To me this is the same as caching new Extent(0.0, 0.0, 0.0, 0.0).

Whether or not to cache an object in JavaScript has everything to do with how often you need it, and nothing to do with how "simple" the object is. There are no "lightweight" JavaScript objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi
Collaborator

Other than my one comment, code and tests are good.

@pjcozzi
Collaborator

Merging.

@pjcozzi pjcozzi merged commit 7f4752a into master
@pjcozzi pjcozzi deleted the terrainImageryCleanup branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 Source/Scene/Imagery.js
@@ -65,10 +65,6 @@ define([
this.image.destroy();
}
- if (typeof this.transformedImage !== 'undefined' && typeof this.transformedImage.destroy !== 'undefined') {
- this.transformedImage.destroy();
- }
-
if (typeof this.texture !== 'undefined' && typeof this.texture.destroy !== 'undefined') {
this.texture.destroy();
}
View
88 Source/Scene/ImageryLayer.js
@@ -87,6 +87,21 @@ define([
* imagery tile for which the contrast is required, and it is expected to return
* the contrast value to use for the tile. The function is executed for every
* frame and for every tile, so it must be fast.
+ * @param {Number|Function} [description.hue=0.0] The hue of this layer. 0.0 uses the unmodified imagery color.
@pjcozzi Collaborator
pjcozzi added a note

Good catch. @bagnell and I forgot to add these. Funny that they made it into the tutorial though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ * This can either be a simple number or a function with the signature
+ * <code>function(frameState, layer, x, y, level)</code>. The function is passed the
+ * current {@link FrameState}, this layer, and the x, y, and level coordinates
+ * of the imagery tile for which the hue is required, and it is expected to return
+ * the contrast value to use for the tile. The function is executed for every
+ * frame and for every tile, so it must be fast.
+ * @param {Number|Function} [description.saturation=1.0] The saturation of this layer. 1.0 uses the unmodified imagery color.
+ * Less than 1.0 reduces the saturation while greater than 1.0 increases it.
+ * This can either be a simple number or a function with the signature
+ * <code>function(frameState, layer, x, y, level)</code>. The function is passed the
+ * current {@link FrameState}, this layer, and the x, y, and level coordinates
+ * of the imagery tile for which the saturation is required, and it is expected to return
+ * the contrast value to use for the tile. The function is executed for every
+ * frame and for every tile, so it must be fast.
* @param {Number|Function} [description.gamma=1.0] The gamma correction to apply to this layer. 1.0 uses the unmodified imagery color.
* This can either be a simple number or a function with the signature
* <code>function(frameState, layer, x, y, level)</code>. The function is passed the
@@ -197,10 +212,6 @@ define([
this._imageryCache = {};
this._texturePool = new TexturePool();
- this._spReproject = undefined;
- this._vaReproject = undefined;
- this._fbReproject = undefined;
-
this._skeletonPlaceholder = new TileImagery(Imagery.createPlaceholder(this));
// The value of the show property on the last update.
@@ -213,10 +224,6 @@ define([
this._isBaseLayer = false;
this._requestImageError = undefined;
-
- this._nonMipmapSampler = undefined;
- this._mipmapSampler = undefined;
- this._reprojectSampler = undefined;
};
/**
@@ -658,9 +665,10 @@ define([
// Use mipmaps if this texture has power-of-two dimensions.
if (CesiumMath.isPowerOfTwo(texture.getWidth()) && CesiumMath.isPowerOfTwo(texture.getHeight())) {
- if (typeof this._mipmapSampler === 'undefined') {
+ var mipmapSampler = context.cache.imageryLayer_mipmapSampler;
+ if (typeof mipmapSampler === 'undefined') {
var maximumSupportedAnisotropy = context.getMaximumTextureFilterAnisotropy();
- this._mipmapSampler = context.createSampler({
+ mipmapSampler = context.cache.imageryLayer_mipmapSampler = context.createSampler({
@pjcozzi Collaborator
pjcozzi added a note

Why cache these at all? This is a light object. See context.prototype.createSampler If anything, createSampler could return cached samplers down the road when WebGL has separate, potentially immutable, sampler and texture objects like Direct3D and OpenGL.

@kring Collaborator
kring added a note

If I didn't cache them, I'd still be creating a bunch of unnecessary objects. And the fact that they're lightweight is an implementation detail of Context, no?

@pjcozzi Collaborator
pjcozzi added a note

To me this is the same as caching new Extent(0.0, 0.0, 0.0, 0.0). Let's disagree and commit. Onward. :+1:

If I change createSampler later, I'll update this.

@mramato Collaborator
mramato added a note

To me this is the same as caching new Extent(0.0, 0.0, 0.0, 0.0).

Whether or not to cache an object in JavaScript has everything to do with how often you need it, and nothing to do with how "simple" the object is. There are no "lightweight" JavaScript objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
wrapS : TextureWrap.CLAMP,
wrapT : TextureWrap.CLAMP,
minificationFilter : TextureMinificationFilter.LINEAR_MIPMAP_LINEAR,
@@ -669,17 +677,18 @@ define([
});
}
texture.generateMipmap(MipmapHint.NICEST);
- texture.setSampler(this._mipmapSampler);
+ texture.setSampler(mipmapSampler);
} else {
- if (typeof this._nonMipmapSampler === 'undefined') {
- this._nonMipmapSampler = context.createSampler({
+ var nonMipmapSampler = context.cache.imageryLayer_nonMipmapSampler;
+ if (typeof nonMipmapSampler === 'undefined') {
+ nonMipmapSampler = context.cache.imageryLayer_nonMipmapSampler = context.createSampler({
wrapS : TextureWrap.CLAMP,
wrapT : TextureWrap.CLAMP,
minificationFilter : TextureMinificationFilter.LINEAR,
magnificationFilter : TextureMagnificationFilter.LINEAR
});
}
- texture.setSampler(this._nonMipmapSampler);
+ texture.setSampler(nonMipmapSampler);
}
imagery.state = ImageryState.READY;
@@ -742,9 +751,30 @@ define([
var float32ArrayScratch = typeof Float32Array === 'undefined' ? undefined : new Float32Array(1);
function reprojectToGeographic(imageryLayer, context, texture, extent) {
- if (typeof imageryLayer._fbReproject === 'undefined') {
- imageryLayer._fbReproject = context.createFramebuffer();
- imageryLayer._fbReproject.destroyAttachments = false;
+ var reproject = context.cache.imageryLayer_reproject;
+
+ if (typeof reproject === 'undefined') {
+ reproject = context.cache.imageryLayer_reproject = {
+ framebuffer : undefined,
+ vertexArray : undefined,
+ shaderProgram : undefined,
+ renderState : undefined,
+ sampler : undefined,
+ destroy : function() {
+ if (typeof this.framebuffer !== 'undefined') {
+ this.frameBuffer.destroy();
+ }
+ if (typeof this.vertexArray !== 'undefined') {
+ this.vertexArray.destroy();
+ }
+ if (typeof this.shaderProgram !== 'undefined') {
+ this.shaderProgram.destroy();
+ }
+ }
+ };
+
+ reproject.framebuffer = context.createFramebuffer();
+ reproject.framebuffer.destroyAttachments = false;
var reprojectMesh = {
attributes : {
@@ -760,23 +790,21 @@ define([
position : 0
};
- imageryLayer._vaReproject = context.createVertexArrayFromMesh({
+ reproject.vertexArray = context.createVertexArrayFromMesh({
mesh : reprojectMesh,
attributeIndices : reprojectAttribInds,
bufferUsage : BufferUsage.STATIC_DRAW
});
- imageryLayer._spReproject = context.getShaderCache().getShaderProgram(
+ reproject.shaderProgram = context.getShaderCache().getShaderProgram(
ReprojectWebMercatorVS,
ReprojectWebMercatorFS,
reprojectAttribInds);
- imageryLayer._rsColor = context.createRenderState();
- }
+ reproject.renderState = context.createRenderState();
- if (typeof imageryLayer._reprojectSampler === 'undefined') {
var maximumSupportedAnisotropy = context.getMaximumTextureFilterAnisotropy();
- imageryLayer._reprojectSampler = context.createSampler({
+ reproject.sampler = context.createSampler({
wrapS : TextureWrap.CLAMP,
wrapT : TextureWrap.CLAMP,
minificationFilter : TextureMinificationFilter.LINEAR,
@@ -785,7 +813,7 @@ define([
});
}
- texture.setSampler(imageryLayer._reprojectSampler);
+ texture.setSampler(reproject.sampler);
var width = texture.getWidth();
var height = texture.getHeight();
@@ -822,14 +850,14 @@ define([
// understand exactly why this is.
outputTexture.generateMipmap(MipmapHint.NICEST);
- imageryLayer._fbReproject.setColorTexture(outputTexture);
+ reproject.framebuffer.setColorTexture(outputTexture);
context.clear(context.createClearState({
- framebuffer : imageryLayer._fbReproject,
+ framebuffer : reproject.framebuffer,
color : new Color(0.0, 0.0, 0.0, 0.0)
}));
- var renderState = imageryLayer._rsColor;
+ var renderState = reproject.renderState;
var viewport = renderState.viewport;
if (typeof viewport === 'undefined') {
viewport = new BoundingRectangle();
@@ -839,11 +867,11 @@ define([
viewport.height = height;
context.draw({
- framebuffer : imageryLayer._fbReproject,
- shaderProgram : imageryLayer._spReproject,
+ framebuffer : reproject.framebuffer,
+ shaderProgram : reproject.shaderProgram,
renderState : renderState,
primitiveType : PrimitiveType.TRIANGLE_FAN,
- vertexArray : imageryLayer._vaReproject,
+ vertexArray : reproject.vertexArray,
uniformMap : uniformMap
});
View
16 Source/Scene/ImageryLayerCollection.js
@@ -296,6 +296,9 @@ define([
*/
ImageryLayerCollection.prototype.raiseToTop = function(layer) {
var index = getLayerIndex(this._layers, layer);
+ if (index === this._layers.length - 1) {
+ return;
+ }
this._layers.splice(index, 1);
this._layers.push(layer);
@@ -316,6 +319,9 @@ define([
*/
ImageryLayerCollection.prototype.lowerToBottom = function(layer) {
var index = getLayerIndex(this._layers, layer);
+ if (index === 0) {
+ return;
+ }
this._layers.splice(index, 1);
this._layers.splice(0, 0, layer);
@@ -383,11 +389,13 @@ define([
}
if (layer.show !== layer._show) {
- layer._show = layer.show;
- if (typeof layersShownOrHidden === 'undefined') {
- layersShownOrHidden = [];
+ if (typeof layer._show !== 'undefined') {
+ if (typeof layersShownOrHidden === 'undefined') {
+ layersShownOrHidden = [];
+ }
+ layersShownOrHidden.push(layer);
}
- layersShownOrHidden.push(layer);
+ layer._show = layer.show;
}
}
View
2  Source/Scene/TileImagery.js
@@ -9,7 +9,7 @@ define(function() {
* @private
*
* @param {Imagery} imagery The imagery tile.
- * @param {Cartesian4} textureCoordinateExtent The texture extent extent of the tile that is covered
+ * @param {Cartesian4} textureCoordinateExtent The texture extent of the tile that is covered
* by the imagery, where X=west, Y=south, Z=east, W=north.
*/
var TileImagery = function(imagery, textureCoordinateExtent) {
View
2  Source/Scene/TileMapServiceImageryProvider.js
@@ -96,7 +96,7 @@ define([
var that = this;
- // Try to load remaing parameters from XML
+ // Try to load remaining parameters from XML
loadXML(url + 'tilemapresource.xml').then(function(xml) {
// Allowing description properties to override XML values
var format = xml.getElementsByTagName('TileFormat')[0];
Something went wrong with that request. Please try again.