Fix slow debug inspector #9004

baothientran · 2020-06-30T16:41:32Z

The issue is that when the scene has debug options turned on, debug commands will be created and executed in each frame. When a debug command is created, the shader of the original command has to be modified and recompiled in each frame as well. It is a very slow operation because the glsl text has to be created from a ShaderSource object. In WebGL 2, it is worse because of the extra step of modernizing the glsl code (for Scene.debugShowFrustums option, you can take a look at the function executeDebugCommand() in Scene.js and ShaderCache.getShaderProgram() in ShaderCache.js)

I created DebugInspector class to cache the debug shader by mapping the original shader id with it. This should speed up the frustum inspector significantly for both WebGL 1 and WebGL 2. If possible, I think I can move all the codes that are used to create debug commands (like debugShowBoundingVolume, etc...) from the Scene.js to DebugInspector.js. That should clean up the Scene.js file, and it is easier to add more state variables later on when debugging without polluting the Scene class.

@lilleyse can you please take a look at it? Please let me know if I should fix anything

…ebugInspector.js

cesium-concierge · 2020-06-30T16:41:36Z

Thanks for the pull request @baothientran!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

lilleyse · 2020-07-07T19:11:33Z

Source/Scene/DebugInspector.js

+    };
+  } else {
+    debugCommand.uniformMap.debugShowCommandsColor = function () {
+      return new Color(1.0, 1.0, 1.0, 1.0);


More concise, and avoids the allocation when retrieving the uniform value every frame.

Suggested change

return new Color(1.0, 1.0, 1.0, 1.0);

return Color.WHITE;

Same comment for debugShowFrustumsColor

lilleyse · 2020-07-07T19:15:39Z

Source/Scene/DebugInspector.js

+  }
+
+  // setup uniform for the shader
+  if (!defined(command.uniformMap) || typeof command.uniformMap !== "object") {


When is the uniform map defined but not an object?

lilleyse · 2020-07-07T19:27:10Z

Source/Scene/DebugInspector.js

+      command._debugColor = Color.fromRandom();
+    }
+
+    debugCommand.uniformMap.debugShowCommandsColor = function () {


This is probably okay since this is debug code, but normally we wouldn't set the uniform function every frame since it allocates a new function object, creates a closure, etc, all of which can add garbage collection pressure. Instead we would add the uniform function on the command once. Maybe you could add debugShowCommandsColor only if the uniform map doesn't already have a uniform with that name.

lilleyse · 2020-07-07T19:34:34Z

Source/Scene/DebugInspector.js

+    var b = command.debugOverlappingFrustums & (1 << 2) ? 1.0 : 0.0;
+
+    debugCommand.uniformMap.debugShowFrustumsColor = function () {
+      return new Color(r, g, b, 1.0);


Similar comment to above. If you end up making a similar change here be sure use command in the closure instead of r, g, b since debugOverlappingFrustums changes every frame. E.g.

if (!defined(debugCommand.uniformMap.debugShowFrustumsColor)) { debugCommand.uniformMap.debugShowFrustumsColor = function () { var r = command.debugOverlappingFrustums & (1 << 0) ? 1.0 : 0.0; var g = command.debugOverlappingFrustums & (1 << 1) ? 1.0 : 0.0; var b = command.debugOverlappingFrustums & (1 << 2) ? 1.0 : 0.0; return new Color(r, g, b, 1.0); } }

Also instead of new Color(r, g, b, 1.0); you could use a scratch color to avoid the allocation. The GL uniforms get updated immediately after the function call so scratches are safe to use.

lilleyse · 2020-07-07T19:35:06Z

Source/Scene/Scene.js

@@ -1964,126 +1963,6 @@ Scene.prototype.isVisible = function (command, cullingVolume, occluder) {
  );
 };

-function getAttributeLocations(shaderProgram) {


Great idea to move this into it's own file and make Scene a little less cluttered

lilleyse · 2020-07-09T02:59:43Z

Source/Scene/DebugInspector.js

+    defined(debugUniformMap.debugShowFrustumsColor) ||
+    defined(debugUniformMap.debugShowFrustumsColor)


Suggested change

defined(debugUniformMap.debugShowFrustumsColor) ||

defined(debugUniformMap.debugShowFrustumsColor)

defined(debugUniformMap.debugShowCommandsColor ) ||

defined(debugUniformMap.debugShowFrustumsColor)

lilleyse · 2020-07-09T02:59:59Z

Source/Scene/DebugInspector.js

+    debugUniformMap = {};
+  } else {
+    debugUniformMap = clone(command.uniformMap);
+    // debugUniformMap = command.uniformMap;


Remove commented code

oh I left a github comment here, but it maybe not shown up to you. My first time using github to contribute to the open source, so I'm still confused about some of its functionalities. Let me paste the comment below and the Sandcastle example for the issue as well:

I try to insert debug uniforms to the current command uniform and stop creating debug uniform closures if they already exist in the command uniform. But there is a problem that when the debug uniform closure captures the command, it may not be the command that is currently being executed. If you un-comment line 87 and have scene.debugShowCommand = true, the cesium billboard's debug color stop changing frequently. I'm not entirely sure, but I think the debug uniform closure only captures one of the billboard's command, which is not correct.

Weird, I didn't see your comment. Maybe it was part of an in-progress review that didn't get submitted?

But there is a problem that when the debug uniform closure captures the command, it may not be the command that is currently being executed

Which suggests the billboard system might be creating a new command every frame but using the same uniform map object. That's interesting...

I prefer the non flickery behavior actually, even though it's different than master. I'd be happy to merge the debugUniformMap = command.uniformMap version.

lilleyse · 2020-07-09T03:01:40Z

Source/Scene/DebugInspector.js

+  if (
+    defined(debugUniformMap.debugShowFrustumsColor) ||
+    defined(debugUniformMap.debugShowFrustumsColor)
+  ) {
+    return debugUniformMap;
+  }


I don't think this condition will actually get hit because debugUniformMap is always cloned from the original command which doesn't contain these properties.

Should clone(command.uniformMap); be command.uniformMap after all? I see it was commented out.

yup this check is initially used for the case debugUniformMap = command.uniformMap. I temporarily left it here since I get a bug when using the shallow copy for uniformMap (I documented the bug in the reply to your comment above)

baothientran · 2020-07-09T15:20:34Z

@lilleyse I've changed clone(uniformMap) to shallow copy. It is ready for review.

lilleyse · 2020-07-09T21:12:18Z

@baothientran did you see #9004 (comment)? I think that's all that's left.

baothientran · 2020-07-09T21:14:00Z

Whoops I missed that. Let me fix it right away

baothientran · 2020-07-09T21:18:27Z

@lilleyse updated

lilleyse · 2020-07-09T21:39:21Z

Thanks @baothientran!

baothientran added 8 commits June 29, 2020 13:39

put debug command into derivedCommands

13bafc8

add uniform for debug shader

012a3d5

Merge branch 'master' into webgl2-slow-inspector

efa6629

create DebugInspector to create debug command

8b7fc5a

modify the uniform for debug shader

d0fd9d2

Merge branch 'master' into webgl2-slow-inspector

0690e3b

remove chrome launch config in launch.json and fix coding style for D…

48aba95

…ebugInspector.js

add check for undefined UniformMap in DebugInspector

a43a090

baothientran requested a review from lilleyse June 30, 2020 16:41

lilleyse reviewed Jul 7, 2020

View reviewed changes

baothientran added 3 commits July 8, 2020 09:27

Merge branch 'master' into webgl2-slow-inspector

32c9652

address PR comments and separate creating uniformMap into a function

3beb6e2

fix mock test for debugShowCommands option

ecdc593

lilleyse reviewed Jul 9, 2020

View reviewed changes

baothientran added 2 commits July 9, 2020 10:19

use shallow copy for debug uniform

c7dde23

remove unused import in DebugInspector

976a31b

fix typo in the debug uniformMap condition in DebugInspector

6a51c2d

lilleyse merged commit 44ac2ff into master Jul 9, 2020

lilleyse deleted the webgl2-slow-inspector branch July 9, 2020 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix slow debug inspector #9004

Fix slow debug inspector #9004

baothientran commented Jun 30, 2020 •

edited

Loading

cesium-concierge commented Jun 30, 2020

lilleyse Jul 7, 2020

lilleyse Jul 7, 2020

lilleyse Jul 7, 2020

lilleyse Jul 7, 2020

lilleyse Jul 7, 2020

lilleyse Jul 7, 2020

lilleyse Jul 9, 2020

lilleyse Jul 9, 2020

baothientran Jul 9, 2020

lilleyse Jul 9, 2020

lilleyse Jul 9, 2020 •

edited

Loading

baothientran Jul 9, 2020

baothientran commented Jul 9, 2020

lilleyse commented Jul 9, 2020

baothientran commented Jul 9, 2020

baothientran commented Jul 9, 2020

lilleyse commented Jul 9, 2020

		defined(debugUniformMap.debugShowFrustumsColor) \|\|
		defined(debugUniformMap.debugShowFrustumsColor)

Fix slow debug inspector #9004

Fix slow debug inspector #9004

Conversation

baothientran commented Jun 30, 2020 • edited Loading

cesium-concierge commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baothientran commented Jul 9, 2020

lilleyse commented Jul 9, 2020

baothientran commented Jul 9, 2020

baothientran commented Jul 9, 2020

lilleyse commented Jul 9, 2020

baothientran commented Jun 30, 2020 •

edited

Loading

lilleyse Jul 9, 2020 •

edited

Loading