-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
3D Tiles - Batch Table Hierarchy #4625
Conversation
Do you mean "style |
CC @pmconne |
Are you happy with these names? Did you have alternatives perhaps borrowed from popular OO languages? |
We can brainstorm in person, but ideally we can expose each feature's full set of properties just like C++ inheritance puts everything into the derived class. If there are real performance or synchronization problems, maybe we would rely on the user being able to get parent classes and read/write those properties. |
Let's roadmap these; I suspect they will be useful, but let's wait until requested and perhaps they will be good beginner issues or external contributions. |
We probably have to detect this in Cesium (and in the validator); for example, we do this for shaders: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Renderer/ShaderSource.js#L129 |
Let's discuss in person when you are ready, but I think it can be done by storing class info in the batch table; we need to think through the memory usage. |
I suspect all properties should be listed there. We may need to update the wording in the spec to clarify this.
Maybe use There could also be a dummy parent (all classes inherit from Let me know if you want to discuss in person.
If it is not hard to implement, I suggest we do it.
This is what I meant above:
We need to think through basically if each derived class gets a copy of its parent's properties or not. It sounds like no in the current implementation. That could be fine, but let's discuss in person to consider all the use cases. |
@@ -369,13 +431,103 @@ define([ | |||
return names; | |||
}; | |||
|
|||
Cesium3DTileBatchTable.prototype.isDerived = function(batchId, className) { |
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.
This is called per feature when applying a style, right?
It could be a hot spot for tilesets with deep feature hierarchies. I don't think it is worth changing now, but perhaps add a PERFORMANCE_IDEA
that we could cache the results in the derived classes if the memory usage is worth it (and replace the string with one string->integer and then integer compares if that is faster).
@@ -338,6 +338,22 @@ define([ | |||
} | |||
val = createRuntimeAst(expression, args[0]); | |||
return new Node(ExpressionNodeType.UNARY, call, val); | |||
} else if (call === 'isClass') { |
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.
Can we reduce some of the duplication here?
Looking good so far. Love the Sandcastle example. |
Ruby has Java has C# is somewhat similar to Java At this point I'm fine with using
Thinking some more, I don't believe this is needed in the GLSL backend. For points at least, it would seem kind of overkill to store class ids and parent ids for every point. If someone needs to represent hierarchies within point clouds they can create a batched point cloud (setting
As discussed offline, we may try something else. Instead of using
Instead this may become a built-in function, like
Yes, the current implementation does not create copies.
Supporting |
All sounds good, except:
So point cloud users would use If not, update CesiumGS/3d-tiles#140 with this limitation. |
More like, the BATCH_ID attribute and batch table hierarchy work together just like it would for batched models. |
If I understand correctly, your saying that the Cesium implementation falls back to the JavaScript backend in this case so it just works? |
Yes |
c3b463e
to
d70d618
Compare
Updated based on the initial round of comments. It now supports multiple parents. In the process I generalized the hierarchy traversal and added back building0 has two parents - zone0 and classifier_new {
"color" : {
"conditions" : [
["isClass('classifier_new') && isClass('classifier_old')", "color('purple')"],
["isClass('classifier_new')", "color('red')"],
["isClass('classifier_old')", "color('blue')"],
["true", "color()"]
]
}
} Sample JSON: {
"HIERARCHY": {
"instancesLength": 36,
"classes": [
{
"name": "doorknob",
"length": 12,
"instances": {
"doorknob_size": [0.3, 0.43, 0.32, 0.2, 0.21, 0.35, 0.3, 0.23, 0.43, 0.32, 0.41, 0.33],
"doorknob_name": ["doorknob0", "doorknob1", "doorknob2", "doorknob3", "doorknob4", "doorknob5", "doorknob6", "doorknob7", "doorknob8", "doorknob9", "doorknob10", "doorknob11"]
}
},
{
"name": "door",
"length": 12,
"instances": {
"door_mass": [10, 11, 14, 7, 8, 12, 3, 6, 3, 5, 9, 10],
"door_width": [1.2, 1.3, 1.21, 1.5, 1.1, 1.15, 1.32, 1.54, 1.8, 2, 2.1, 1.3],
"door_name": ["door0", "door1", "door2", "door3", "door4", "door5", "door6", "door7", "door8", "door9", "door10", "door11"]
}
},
{
"name": "roof",
"length": 4,
"instances": {
"roof_paint": ["red", "blue", "yellow", "green"],
"roof_name": ["roof0", "roof1", "roof2", "roof3"]
}
},
{
"name": "wall",
"length": 4,
"instances": {
"wall_paint": ["pink", "orange", "blue", "gray"],
"wall_windows": [1, 2, 4, 2],
"wall_name": ["wall0", "wall1", "wall2", "wall3"]
}
},
{
"name": "building",
"length": 4,
"instances": {
"building_area": [20, 21.98, 39.3],
"building_name": ["building0", "building1", "building2"]
}
},
{
"name": "zone",
"length": 1,
"instances": {
"zone_buildings": [4],
"zone_name": ["zone0"]
}
},
{
"name": "classifier_new",
"length": 1,
"instances": {
"year": [2000],
"color": ["red"],
"name": ["project"],
"architect": ["architect"]
}
},
{
"name": "classifier_old",
"length": 1,
"instances": {
"description": ["built in 1980"],
"inspection": [2009]
}
}
],
"classIds": [0, 0, 0, 0, 1, 1, 1, 1, 2, 3, 0, 0, 0, 0, 1, 1, 1, 1, 2, 3, 0, 0, 0, 0, 1, 1, 1, 1, 2, 3, 4, 4, 4, 5, 6, 7],
"parentIds": [4, 5, 6, 7, 30, 30, 30, 30, 30, 30, 14, 15, 16, 17, 31, 31, 31, 31, 31, 31, 24, 25, 26, 27, 32, 32, 32, 32, 32, 32, 33, 34, 33, 35, 33, 34, 35],
"parentCounts": [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 2, 3, 0, 0, 0]
},
"height": [0.1, 0.1, 0.1, 0.1, 5, 5, 5, 5, 6, 10, 0.1, 0.1, 0.1, 0.1, 5, 5, 5, 5, 6, 10, 0.1, 0.1, 0.1, 0.1, 5, 5, 5, 5, 6, 10],
"area": [0.2, 0.2, 0.2, 0.2, 10, 10, 10, 10, 12, 20, 0.2, 0.2, 0.2, 0.2, 10, 10, 10, 10, 12, 20, 0.2, 0.2, 0.2, 0.2, 10, 10, 10, 10, 12, 20]
} |
This is ready with the exception of |
Is the array likely to grow very large over time? |
OK, just leave it. We can reevaluate if needed. |
Will aim to do another review tomorrow. |
OK for now since this is only a Cesium API issue. What is the state of this? I would be OK with all classes being readonly in the Cesium API so the only thing that can change is the leaf nodes, which are the actual features. |
|
||
//>>includeStart('debug', pragmas.debug); | ||
function validateHierarchy(hierarchy) { | ||
var stack = scratchStack; |
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.
I would caution against using scratchStack
here since it will have different debug/release usage and could lead to subtle bugs when new code is written. To make the validation code self-contained just use a different scratch for it.
if (defined(propertyValues.typedArray)) { | ||
return getBinaryProperty(propertyValues, indexInClass); | ||
} | ||
return clone(propertyValues[indexInClass], true); |
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.
Here and below, what is the context in which this is call? Is the memory allocation in clone
OK?
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.
I modeled this after getProperty. I believe the idea is that we don't want to make batch table properties that are objects editable via the getProperty
function.
if (!defined(name)) { | ||
throw new DeveloperError('name is required.'); | ||
} | ||
//>>includeEnd('debug'); | ||
|
||
var json = this.batchTableJson; | ||
return defined(json) && defined(json[name]); | ||
if (defined(json) && defined(json[name])) { |
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.
Style but why not write this entire block as
return (defined(json) && defined(json[name]) || (defined(this._batchTableHierarchy) && hasPropertyInHierarchy(this, batchId, name));
Or at least change the last block to just:
return (defined(this._batchTableHierarchy) && hasPropertyInHierarchy(this, batchId, name));
@@ -55,7 +55,7 @@ define([ | |||
asin : Math.asin, | |||
atan : Math.atan, | |||
radians : CesiumMath.toRadians, | |||
degrees : CesiumMath.toDegrees | |||
degrees : CesiumMath.toDegrees, |
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.
Extra comma?
|
||
addStyle('No style', {}); | ||
|
||
addStyle('Color by building', { |
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.
Can you add an example using getClassName
?
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.
Also, I think this example needs more context; it is a bit hard to hack on without knowing what the semantic hierarchy looks like. Perhaps there should be comments in this example with similar info to what you put in the opening comment of this PR.
//>>includeEnd('debug'); | ||
val = createRuntimeAst(expression, args[0]); | ||
return new Node(ExpressionNodeType.UNARY, call, val); | ||
} else if (call === 'getClassName') { |
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.
Throughout, should this be called getExactClassName
?
Looking good, just those comments! |
Not too large. At most the size of the array will equal the tile with the most instances.
This might be a good idea... I'm okay with the keeping the code as it is now since it gives a little bit more flexibility within the tile. But if others are confused that setting hierarchy properties don't apply to different tiles then it may be worth making it read only. |
Updated. |
OK, let's go with readonly. Freeze it too. Will this remove the |
That's actually kind of a lot - could be, for example, 50,000. Assuming there is no reasonable way around this, please add a comment about the potential bounds. |
The whole hierarchy can't be frozen since you are still allowed to set properties on an instance's own class. Clone still needs to exist just like it does with the normal |
This is ready. |
Very nice, looking forward to the spec updates |
For #3241
Spec: CesiumGS/3d-tiles#66
This adds initial support for hierarchies in the batch table to help reduce the amount of duplicate metadata. More details behind this are in the linked spec issue. The
3D Tiles Hierarchy
demo has some example stylesNew features:
Cesium API:
Cesium3DTileFeature.isClass(className)
- returns whether the feature's class isclassName
. This does not check ancestor classes.Cesium3DTileFeature.isDerived(className)
- returns whether the feature derives fromclassName
. This checks the feature's class and ancestor classes.Styling language:
isClass
- same as above -isClass('door')
- styles just doorsisDerived
- same as above -isDerived('door')
- styles doors and doorknobs"show" : "${building_name} === 'building0'"
will stylebuilding0
and all descendants ofbuilding0
Todo:
Cesium3DTileBatchTable.hasProperty
,Batched3DModel3DTileContent.hasProperty
, etc to take the hierarchy into account. It will need to accept abatchId
argument because not all features have the same properties anymore. Or this doesn't change and just continues to check only base-level batch table properties (likearea
andheight
in the sample below).getPropertyNames
getFeaturesByProperty(propertyName, value)
getFeaturesByClass(className)
isClass
andisDerived
Questions:
tileset.json
properties
section?parentId
of-1
signifies no parent. However this makes it harder to store the parentIds in binary unless the only allowedcomponentType
areSHORT
orINT
.${CLASS}
property for styling? This would allow regular expressions against the class name. Or isisClass
good enough?feature.setProperty('building_name', 'new_name')
be allowed? Currently this is supported in the code and is probably ok because otherwise we would need some other interface to represent abstract instances - like an analog toCesium3DTileFeature
, and that can get complicated.Sample hierarchy:
Doorknobs are children of doors
Sample batch table json:
Same example using the batch table binary: