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

I3dm updates #4101

Merged
merged 5 commits into from
Aug 22, 2016
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
2 changes: 2 additions & 0 deletions Apps/Sandcastle/gallery/3D Tiles.html
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
name : 'Batched', url : '../../../Specs/Data/Cesium3DTiles/Batched/BatchedWithBatchTable/'
}, {
name : 'Instanced', url : '../../../Specs/Data/Cesium3DTiles/Instanced/InstancedWithBatchTable/'
}, {
name : 'Instanced/Orientation', url : '../../../Specs/Data/Cesium3DTiles/Instanced/InstancedOrientationWithBatchTable/'
}, {
name : 'Composite', url : '../../../Specs/Data/Cesium3DTiles/Composite/Composite/'
}, {
Expand Down
85 changes: 85 additions & 0 deletions Source/Scene/Cesium3DTileFeatureTableResources.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/*global define*/
define([
Copy link
Contributor

Choose a reason for hiding this comment

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

@bagnell did you look at this file to see if you can use it for vector data?

'../Core/ComponentDatatype',
'../Core/defaultValue',
'../Core/defined',
'../Core/DeveloperError'
], function(
ComponentDatatype,
defaultValue,
defined,
DeveloperError) {
'use strict';

/**
* @private
*/
function Cesium3DTileFeatureTableResources(featureTableJSON, featureTableBinary) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now the approach of generating a JS array that holds onto the property's binary data just seems too expensive. I think it would be easier if this class stored a TypedArray view for each property, and then when getting an instance's property it just indexes into the typed array. To avoid allocations, the getter can take an optional results array parameter.

This is also make it easier to work with Points3DTileContent which wants TypedArrays for the color and position properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see this comment: #4101 (comment) kind of alludes to that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lasalvavida can we write unit tests that test this file in isolation without an entire tile?

this.json = featureTableJSON;
this.buffer = featureTableBinary;
this._cachedArrayBufferViews = {};
this.featuresLength = 0;
}

Cesium3DTileFeatureTableResources.prototype.getTypedArrayForSemantic = function(semantic, byteOffset, componentType, count, featureSize) {
//>>includeStart('debug', pragmas.debug);
if (!defined(byteOffset)) {
throw new DeveloperError('byteOffset must be defined to read from binary data for semantic: ' + semantic);
}
if (!defined(componentType)) {
throw new DeveloperError('componentType must be defined to read from binary data for semantic: ' + semantic);
}
if (!defined(count)) {
throw new DeveloperError('count must be defined to read from binary data for semantic: ' + semantic);
}
//>>includeEnd('debug');
var cachedArrayBufferViews = this._cachedArrayBufferViews;
var arrayBuffer = cachedArrayBufferViews[semantic];
if (!defined(arrayBuffer)) {
arrayBuffer = ComponentDatatype.createArrayBufferView(componentType, this.buffer.buffer, this.buffer.byteOffset + byteOffset, count * featureSize);
cachedArrayBufferViews[semantic] = arrayBuffer;
}
return arrayBuffer;
};

Cesium3DTileFeatureTableResources.prototype.getGlobalProperty = function(semantic, componentType, count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that "global" is the best word here; it's not really about scope. It just means apply to all instances, not individual instances. I don't have anything better right now though. Will let you know if I think of something.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be cleaner to split this implementation into separate global and non-global getters. There may be some duplication, but the logic and parameters will be decoupled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would a global property ever need to be stored in binary? If not, this function could just return the JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not; I think that's probably better for making the implementation more streamlined

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should assume it will always be JSON. A global property could be a big array, even if it isn't per-feature data, so we shouldn't make the spec this limited unless there is a significant implementation reason, which I do not see here since we already need to handle binary for the other properties.

var jsonValue = this.json[semantic];
if (defined(jsonValue)) {
var byteOffset = jsonValue.byteOffset;
if (defined(byteOffset)) {
// This is a reference to the binary
count = defaultValue(count, 1);
var typedArray = this.getTypedArrayForSemantic(semantic, byteOffset, componentType, count, 1);
var subArray = typedArray.subarray(0, count);
Copy link
Contributor

Choose a reason for hiding this comment

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

@lilleyse can you confirm that this is a fast operation, e.g., just a pointer splice, not a copy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's fine, it creates a view of the array buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, is the subarray needed in this case? The typedArray is equal to subArray right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, it may be cached.

if (subArray.length === 1) {
return subArray[0];
}
return subArray;
}
}
return jsonValue;
};

Cesium3DTileFeatureTableResources.prototype.getProperty = function(semantic, featureId, componentType, featureSize) {
var jsonValue = this.json[semantic];
if (defined(jsonValue)) {
var byteOffset = jsonValue.byteOffset;
if (defined(byteOffset)) {
// This is a reference to the binary
featureSize = defaultValue(featureSize, 1);
var typedArray = this.getTypedArrayForSemantic(semantic, byteOffset, componentType, this.featuresLength, featureSize);
var subArray = typedArray.subarray(featureId * featureSize, featureId * featureSize + featureSize);
if (subArray.length === 1) {
return subArray[0];
}
return subArray;
}
}
if (Array.isArray(jsonValue)) {
return jsonValue.slice(featureId * featureSize, featureId * featureSize + featureSize);
}
return jsonValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this return jsonValue[featureId]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should, my mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the comment above, it should return jsonValue[featureId] if jsonValue is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I missed that.

};

return Cesium3DTileFeatureTableResources;
});
Loading