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

DebugTilesPlugin: optimize performance and make it toggleable #885

Merged
merged 4 commits into from
Dec 22, 2024

Conversation

sguimmara
Copy link
Contributor

@sguimmara sguimmara commented Dec 20, 2024

This PR optimizes the DebugTilesPlugin by reducing its overhead, an makes it toggleable.

fix #647

Tree traversal optimizations

This PR optimizes (some) tree traversals by using a stack-based approach, rather than a recursive approach, that reduces overhead of recursion.

traverseSet() is modified from a recursive approach to a stack-based approach. toggleTiles() now uses this version.

Note: other functions in traverseFunctions.js might also benefit from using this approach, but the migration is not as easy as for toggleTiles(), as the recursion logic is entangled with the callback logic.

Disable/Enable the DebugTilesPlugin dynamically

Add the enabled property on the plugin. When toggled on, the plugin initializes its state from the current tile hierarchy. When toggled off, the existing helpers are removed, and all event listeners do nothing (except for 'dispose-model').

@sguimmara
Copy link
Contributor Author

Before continuing further, I would like your opinion on this @gkjohnson.

The biggest performance hit of DebugTilesPlugin is the tree traversal(s) in _initExtremes(), because the traversals preprocess all tiles in the tileset, which is not necessary for the needs of _initExtremes().

In one of Giro3D examples, we have a massive tileset with a lot of nested tilesets, which means that the preprocessing happens a lot, rather than once (which would be the case with a single root tileset.json).

@sguimmara sguimmara force-pushed the debug-tiles-plugins-toggling branch 2 times, most recently from b79d5d0 to 2286da8 Compare December 20, 2024 13:11
@sguimmara sguimmara force-pushed the debug-tiles-plugins-toggling branch 4 times, most recently from 4aa6430 to 713dc2a Compare December 20, 2024 14:09
@sguimmara sguimmara marked this pull request as ready for review December 20, 2024 14:21
@sguimmara
Copy link
Contributor Author

I actually ended up implementing the toggling itself, so it's ready for review

Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Amazing! Thank you. I added a few comments but this should make the plugin a lot nicer to use.

src/base/traverseFunctions.js Outdated Show resolved Hide resolved
test/traverseFunctions.test.js Show resolved Hide resolved
src/plugins/three/DebugTilesPlugin.js Outdated Show resolved Hide resolved

traverseSet( this.tiles.root, null, ( tile, parent, depth ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use traverseSet rather than tiles.traverse?


} else {

this._removeHelpers();
Copy link
Contributor

Choose a reason for hiding this comment

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

Material changes are done based on the colorMode in the _onUpdateAfter function so we'll also want to make sure we revert the material changes here. This is done in the dispose function like so:

// reset all materials
this.colorMode = NONE;
this._onUpdateAfter();

Now that I'm looking at it I'm wondering if we should just call this.dispose when enabled is toggled to "false" and then call init again when it's toggled to "true". Then the plugin can clean up after itself in a consistent way, unhook any events so there's no added overhead. That may be a smaller change.

@sguimmara sguimmara force-pushed the debug-tiles-plugins-toggling branch 3 times, most recently from 4a806ac to cb23fdc Compare December 21, 2024 10:44
@sguimmara
Copy link
Contributor Author

@gkjohnson I updated the PR with the following changes:

  • removed the commit about toggleTiles that would use the non-recursive traversal
  • simplified the toggling logic by simpling calling init() and dispose() with enabling and disabling
  • added various checks (for example, dispose() does nothing if the plugin is not initialized, otherwise it would try to remove objects that do not exist and raise errors)
  • added a chekbox in the main example to enable/disable the plugin:
image

Copy link
Contributor

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Thanks! Last couple comments and then this looks ready to merge. I'll have to think about how to deal with the traverseSet issue - would be nice to not have to import that utility function separately.

@@ -108,31 +141,48 @@ export class DebugTilesPlugin {
// register events
this._onLoadTileSetCB = () => {

this._initExtremes();
if ( this.enabled ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these gates still needed here? These events will not be added to the tiles object if the flag is not enabled.


};

this._onDisposeModelCB = ( { tile } ) => {

// Note that we want to cleanup memory even if the plugin is disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems out of date - since we remove the events when the plugin is disabled this function will no longer be called.

@gkjohnson gkjohnson added this to the v0.3.46 milestone Dec 22, 2024
sguimmara and others added 4 commits December 22, 2024 09:46
For deep hierarchy of nested tilesets, recursive traversal has a severe
performance cost. Use a stack-based, depth first traversal instead.
We don't need the tiles to be preprocessed in this function, which has
a severe performance cost.
@sguimmara
Copy link
Contributor Author

@gkjohnson seems it also solves #484

@gkjohnson
Copy link
Contributor

Excellent, thank you! I'll update the docs to add the field.

@gkjohnson gkjohnson merged commit ad3b888 into NASA-AMMOS:master Dec 22, 2024
2 checks passed
@sguimmara sguimmara deleted the debug-tiles-plugins-toggling branch December 24, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DebugTilesPlugin: Add option for enabling / disabling plugin to toggle performance
2 participants