Add wireframe debug option for 3D Tilesets #4844
Conversation
|
Looks good at quick glance! @lilleyse please review and merge. |
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This can be combined with updateDerivedCommandsShadows and the function renamed to just updateDerivedCommands.
| @@ -939,6 +941,27 @@ defineSuite([ | |||
| }); | |||
| }); | |||
|
|
|||
| it('debugWireframe', function() { | |||
| // More precise test is in Cesium3DTileBatchTableSpec | |||
| scene.renderForSpecs(); | ||
| var commands = scene.frameState.commandList; | ||
| var i; | ||
| for (i = 0; i < commands.length; ++i) { |
There was a problem hiding this comment.
Typically we factor out commands.length to its own variable for a slight performance gain. It won't matter for tests, but is a good habit for consistency purposes.
| @@ -343,6 +343,10 @@ define([ | |||
| this.batchTable.setAllColor(color); | |||
| }; | |||
|
|
|||
| Batched3DModel3DTileContent.prototype.applyWireframe = function(enabled) { | |||
| this._model.debugWireframe = enabled; | |||
| }; | |||
There was a problem hiding this comment.
Instead of adding this function to every content type, it would be shorter to model debugWireframe after how shadows are set in Instanced3DModel3DTileContent and Batched3DModel3DTileContent.
It looks like you modeled it after applyDebugSettings. The reason that one is its own function is because it can be a quite heavy operation (this.batchTable.setAllColor(color);) that we wouldn't want to call every frame. For the case of shadows and debugWireframe though there is hardly any impact to do the simple updates every frame.
|
Updated. Thanks for pointing out why we update for debugColorizeTiles more conservatively. |
|
@austinEng in the future when you open a pull request, please include a comment like "Fixes #3830" so that reviewers can easily check out the issue and GitHub will automatically close it when the PR is merged (unless the issue includes a tasklist with unchecked items). |
| @@ -342,7 +343,7 @@ define([ | |||
| color = enabled ? color : Color.WHITE; | |||
| this.batchTable.setAllColor(color); | |||
| }; | |||
|
|
|||
| @@ -325,6 +325,7 @@ define([ | |||
| this._debugViewerRequestVolume = undefined; | |||
| this._debugColor = new Color.fromRandom({ alpha : 1.0 }); | |||
| this._debugColorizeTiles = false; | |||
| this._debugWireframe = false; | |||
| @@ -123,6 +123,7 @@ define([ | |||
| */ | |||
| Tileset3DTileContent.prototype.applyDebugSettings = function(enabled, color) { | |||
| }; | |||
|
|
|||
|
Updated |
|
Thanks! |
No description provided.