-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add support to check children bounds for visibility before selecting a tile #4978
Conversation
Part of #3241 |
* @param {Boolean} [force=false] Whether to always evaluate support for the childrenWithinParent optimization | ||
* @returns {Boolean} Whether the childrenWithinParent optimization is supported. | ||
*/ | ||
Cesium3DTileOptimizations.prototype.checkChildrenWithinParent = function(tile, evaluate, force) { |
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.
Depending on your tests, we may end up removing this, e.g., if the new CPU overhead is trivial and/or it is a win for most datasets.
If either of the above is true, we may just do it all the time and even avoid include hints in the tileset.
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 think we can probably just check this every time. In practice, we only need to evaluate it once per tile (unless tiles move relative to their parent?). So really, this is just a small one-time cost on load.
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.
It's not about load time performance, it's about client-side complexity.
My point is that writing a general test for this taking into account all the different bounding volume types (basically it is CSG union) could be a lot of work to write, test, etc. Maybe there are some heuristics to simplify this by putting boxes around regions and using a cost function dependent on the number of children.
I dunno, could be complicated.
So we might just skip this completely on the client if the tileset contains a hint to do it or not, or if always checking the children is fast/useful enough.
In addition to testing terrain, cities, and design models, @lilleyse can you provide a massive point cloud dataset for testing? |
Source/Scene/Cesium3DTileset.js
Outdated
if ((sse <= maximumScreenSpaceError) || (childrenLength === 0)) { | ||
// This tile meets the SSE so add its commands. | ||
// Select tile if it's a leaf (childrenLength === 0) | ||
selectTile(tileset, t, fullyVisible, frameState); | ||
if (useChildrenBoundUnion) { |
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.
Are you sure this is the right place to add this code? I was expecting a place that use to have
checkThisTilesVolume
to be replaced with
checkTheUnionOfTheTilesChildsVolumes
Source/Scene/Cesium3DTileset.js
Outdated
@@ -1311,50 +1372,47 @@ define([ | |||
} | |||
|
|||
if (!allChildrenLoaded) { | |||
// Tile does not meet SSE. Add its commands since it is the best we have and request its children. | |||
selectTile(tileset, t, fullyVisible, frameState); | |||
childrenVisibility = computeChildrenVisibility(t, frameState, false); |
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.
Why isn't useChildrenBoundUnion
called before computeChildrenVisibility
to save the CPU overhead when it isn't needed?
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.
Thanks, good catch
c8e7d43
to
721ca18
Compare
Source/Scene/Cesium3DTile.js
Outdated
* | ||
* @private | ||
*/ | ||
this.childrenVisibility = 1; |
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.
Instead of assigning to 1
, ChildrenVisibilityFlags
should be in its own file (it can be @private
) and declared like other Cesium enums. For example, see Cesium3DTileContentState.js.
* | ||
* Flag denoting support for a 3D Tiles optimization | ||
*/ | ||
var Cesium3DTileOptimizationFlag = { |
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 should also be @private
in its own file.
Also, we generally don't suffix enums with Flag
or Flags
. Perhaps there are better names for these.
* Flag denoting support for a 3D Tiles optimization | ||
*/ | ||
var Cesium3DTileOptimizationFlag = { | ||
UNSUPPORTED: 0, |
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.
Perhaps rename these to:
NOT_COMPUTED
USE_OPTIMIZATION
SKIP_OPTIMIZATION
In Cesium, "supported" usually refers to WebGL features.
* }) | ||
* }); | ||
*/ | ||
function Cesium3DTileOptimizations(options) { |
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.
In the doc, this should be @private
.
|
||
/** | ||
* Defines optional optimization flags for a {@link Cesium3DTileset}. | ||
* Optimizations marked as Cesium3DTileOptimizations.Flags.SUPPORTED or Cesium3DTileOptimizations.Flags.UNSUPPORTED |
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.
Use {@link }
when referring to enums.
For more, see the doc guide.
Source/Scene/Cesium3DTileset.js
Outdated
@@ -270,7 +274,8 @@ define([ | |||
lastColor : new Cesium3DTilesetStatistics(), | |||
lastPick : new Cesium3DTilesetStatistics() | |||
}; | |||
|
|||
|
|||
this._optimizations = defaultValue(options.optimizations, new Cesium3DTileOptimizations()); |
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.
Just remove this for now until we decide on CesiumGS/3d-tiles#176
evaluate = defaultValue(evaluate, false); | ||
force = defaultValue(force, false); | ||
|
||
if ((this.childrenWithinParent === Cesium3DTileOptimizations.Flags.INDETERMINATE && evaluate) || force) { |
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.
Just a reminder to comment this function as we discussed offline.
Source/Scene/Cesium3DTileset.js
Outdated
@@ -1285,11 +1336,21 @@ define([ | |||
// With replacement refinement, if the tile's SSE | |||
// is not sufficient, its children (or ancestors) are | |||
// rendered instead | |||
var useChildrenBoundUnion = t._optimizations.checkChildrenWithinParent(t, 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.
This workflow is a bit awkward. Can we just compute this on load and then just check a boolean property here?
Longer-term, this is the type of processing that would be in a web worker when a tile is downloaded and before it is ready to render.
Please unit test this carefully including cases with request volumes. |
7cb5b40
to
cd0ff90
Compare
* @alias Cesium3DTileOptimizations | ||
* @constructor | ||
*/ | ||
function Cesium3DTileOptimizations(options) { |
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.
To avoid some indirection at runtime (and likely cache misses), consider replacing with with a boolean property on the tile. So file would just contain a single function for checkChildrenWithinParent
.
In time, the boolean would probably become a bitmask with many booleans for different optimizations and tiles would more cohesively fit into cache lines (I'm not saying that they do today, but this extra indirection does not help the case).
Source/Scene/Cesium3DTileset.js
Outdated
@@ -270,7 +276,7 @@ define([ | |||
lastColor : new Cesium3DTilesetStatistics(), | |||
lastPick : new Cesium3DTilesetStatistics() | |||
}; | |||
|
|||
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 whitespace.
@lilleyse can you review this? |
c607a56
to
61c9567
Compare
61c9567
to
9072938
Compare
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.
The code here looks good and organized. I tested with a few different tilesets and haven't noticed any issues.
'../Core/freezeObject', | ||
'./Cesium3DTileOptimizationHint', | ||
'./TileOrientedBoundingBox', | ||
'./TileBoundingRegion' |
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.
Flip these two (alphabetical ordering)
'use strict'; | ||
|
||
/** | ||
* Utiility functions for computing optimization hints for a {@link Cesium3DTileset}. |
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.
Utiility typo
} | ||
} | ||
|
||
return tile._optimChildrenWithinParent === Cesium3DTileOptimizationHint.USE_OPTIMIZATION ? true : false; |
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.
Why not just return tile._optimChildrenWithinParent === Cesium3DTileOptimizationHint.USE_OPTIMIZATION;
* spheres are not supported for the reason that the child bounds can very often be partially outside of the parent bounds. | ||
* | ||
* @param {Cesium3DTile} tile The tile to check. | ||
* @param {Boolean} [evaluate=false] Whether to evaluate support if support for the childrenWithinParent optimization is Cesium3DTileOptimizations.Hints.NOT_COMPUTED |
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.
Fix wording here.
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.
Cesium3DTileOptimizations.Hints
should be Cesium3DTileOptimizationHints
*/ | ||
Cesium3DTileOptimizations.checkChildrenWithinParent = function(tile, evaluate, force) { | ||
evaluate = defaultValue(evaluate, false); | ||
force = defaultValue(force, false); |
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.
Throw DeveloperError if tile
is undefined.
} | ||
} else { | ||
selectTile(tileset, t, fullyVisible, frameState); | ||
} |
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.
Given these updates, it may be cleaner to separate out the childrenLength === 0
and put it before the sse <= maximumScreenSpaceError
check.
Source/Scene/Cesium3DTileset.js
Outdated
if (useChildrenBoundUnion) { | ||
childrenVisibility = computeChildrenVisibility(t, frameState, false); | ||
} | ||
if (!useChildrenBoundUnion || (childrenVisibility & Cesium3DTileChildrenVisibility.VISIBLE) || childrenLength === 0) { |
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.
No need to check childrenLength === 0
here.
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.
Same applies to a few other areas.
Specs/Scene/Cesium3DTilesetSpec.js
Outdated
@@ -840,6 +845,158 @@ defineSuite([ | |||
}); | |||
}); | |||
|
|||
describe('children bound union optimization', function() { | |||
it ('does not select visible tiles with invisible children', function() { |
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.
it (
-> it(
Specs/Scene/Cesium3DTilesetSpec.js
Outdated
it ('does not select visible tiles not meeting SSE with visible children', function() { | ||
return Cesium3DTilesTester.loadTileset(scene, tilesetReplacementWithViewerRequestVolumeUrl).then(function(tileset) { | ||
var root = tileset._root; | ||
var child_root = root.children[0]; |
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.
Use camel case instead.
@@ -0,0 +1,174 @@ | |||
{ |
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 open a PR into https:/AnalyticalGraphicsInc/3d-tiles-tools/tree/master/generator that adds this tileset?
I don't actually have a massive point cloud that does replacement refinement... |
OK, we should be fine here. @austinEng can you merge in @lilleyse please merge when ready and check this off the roadmap, #3241 |
@lilleyse updated |
Source/Scene/Cesium3DTileset.js
Outdated
// This tile meets the SSE so add its commands. | ||
if (useChildrenBoundUnion) { | ||
childrenVisibility = computeChildrenVisibility(t, frameState, false); | ||
if ((childrenVisibility & Cesium3DTileChildrenVisibility.VISIBLE) || childrenLength === 0) { |
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.
Remove childrenLength === 0
Source/Scene/Cesium3DTileset.js
Outdated
// and this tile can refine to them. | ||
touch(tileset, child, outOfCore); | ||
if (outOfCore) { | ||
for (k = 0; (k < childrenLength) && t.canRequestContent(); ++k) { |
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 area was probably missed in the merge conflict resolution, but make sure to fix it with the changes from here: https:/AnalyticalGraphicsInc/cesium/pull/5008/files
@lilleyse updated |
Alright, everything looks good here now. |
Todo: