More 3D Tiles inspector cleanup #5244
Conversation
| * @param {Cesium3DTileFeature} feature The feature | ||
| * @returns {String} The url to that feature's tile | ||
| */ | ||
| Cesium3DTilesInspectorViewModel.prototype.getFeatureUrl = function(feature) { |
There was a problem hiding this comment.
Is there a reason we are doing this here rather than exposing url on the feature? (Or maybe even exposing content on the feature and then url on the content? url on the feature seems like the minimal desired change here.
There was a problem hiding this comment.
Maybe I misunderstood, but this is what I thought you recommended that I do
There was a problem hiding this comment.
Sorry if there was miscommunication, I would much rather see feature.content.url, Since Cesium3DTileContent takes url as one of it's constructor parameters, we should definitely expose it, same for Cesium3DTileFeature. In general, a class should always expose the core components that it takes in it's primary constructor as properties.
|
Just the one comment. |
|
@mramato ready |
Source/Scene/Cesium3DTileContent.js
Outdated
| }, | ||
|
|
||
| /** | ||
| * Gets the url of the tile's content |
There was a problem hiding this comment.
We end doc comments with a period.
| }, | ||
|
|
||
| /** | ||
| * Part of the {@link Cesium3DTileContent} interface. |
There was a problem hiding this comment.
This should just be the same exact doc as Cesium3DTileContent (too bad we can't use @inheritdoc since it's not real inheritance).
There was a problem hiding this comment.
I copied what the doc is for the rest of the properties in this file
There was a problem hiding this comment.
OK, so they are wrong too, we'll fix that when 3D-tiles get PRed into master but no need to add to it.
Source/Scene/Cesium3DTileContent.js
Outdated
| * @type {String} | ||
| * @readonly | ||
| * | ||
| * @private |
There was a problem hiding this comment.
Why is this private? We plan on using it externally so it should be public.
Source/Scene/Cesium3DTileFeature.js
Outdated
| }, | ||
|
|
||
| /** | ||
| * Gets the feature content |
|
Just those couple of additional comments. |
|
@mramato ready |
hpinkos
force-pushed
the
3dtilesinspectorcleanup
branch
from
d2adc66 to
502f00f
Compare
|
Okay, actually ready now @mramato |
|
Thanks @hpinkos! I'll merge as soon as this goes green. |
Made the following changes to
Cesium3DTilesInspectorViewModel:propertiesTextstring withproperties, which is an array of strings_featureobservableperformanceContainerparameter