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

Add description to DrawCommand #12119

Closed
wants to merge 2 commits into from
Closed

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Aug 11, 2024

I recently tried to address some features/issues that are touching the innermost guts of the CesiumJS rendering system (#12075 , #12105, #12112 ...). And I found myself spending way more than 90% of my time not with implementing the feature or fixing the bug, but with reading, reading, reading, and debugging, debugging, debugging. (It's closer to 100%, actually...). This is not really productive.

One reason for that is that it is essentially impossible to figure out what is actually happening during "rendering": Draw commands are "created", "derived", and "updated", and eventually, some of them are "executed".

(Each of the terms in quotes should have a meaning of which it should be possible to describe it in a /** Single-line comment */, but certainly none of them does. I tried to shrug off the lack of documentation, but this is one reason for bugs, and one of the reasons why it takes so much time to fix the bugs. Ignoring this problem embiggens it in the long run)

This PR adds a description to the DrawCommand. Whenever a DrawCommand is created, a description is passed in that tries to capture some of the context about what that draw command is. (Right now, it's a first shot - one could always add further details). Whenever a draw command is "derived" with DrawCommand.shallowClone, then a suffix is passed in that will be appended to the description.

Of course, this is just a crutch. It should not be necessary to debug-step through the code in order to trace one particular draw command. Doing what is necessary to achieve a state where this is not necessary is beyond what a single person can accomplish in reasonable time. But when it is necessary, then this simple string could already be helpful - for example, to figure out which primitives have wrong bounding volumes (c.f. #12108):

Cesium Draw Command Description 0001

So I'm just opening this as a suggestion.

(I'll probably do my debugging locally based on this state, try to implement the fix, and in doubt, carve out the 'isolated' fix into a PR that does not contain the changes from this PR...)

Testing plan

Maybe debug-step though the code and track what is happening in terms of commands that are executed...?

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @javagl!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Aug 12, 2024

@lilleyse Would you mind looking over this approach and let us know what your thoughts are before we proceed with a code review? I'm curious whether this is typical or if it has any larger implecations.

@javagl
Copy link
Contributor Author

javagl commented Aug 13, 2024

I'd like to emphasize that this is something "weaker than a 'proposal'".

The point is that figuring out which draw commands are executed can be difficult, and certain assumptions can not be made (e.g. just because executeCommand receives a command does not mean that it executes this command). I think that some mechanism for identifying them may be useful, and this PR is only one way. (There may be downsides to what is implemented here - like too many string concatenations or so...? Well, likely not the biggest problem, but maybe there are others...)

@lilleyse
Copy link
Contributor

description seems pretty useful for debugging, I'm just not sure if it should be so tightly integrated with shallowClone.

@javagl
Copy link
Contributor Author

javagl commented Aug 14, 2024

On the one hand, I think that the description without the part that is appended via the 'shallowClone' could already be useful. On the other hand, I also don't like what you probably meant with "tight integration", namely, that one has to pass that 'suffix' to the shallowClone method everywhere. On the third hand (..?!..), this could sometimes be exactly the part that is relevant.

For example, looking at the "Shadows" sandcastle with this branch, some log output from a single frame prints

Executing  skyBox.command
Executing  skyAtmosphere._command.logDepth
Executing  tileProvider._drawCommands[0].logDepth.receive
Executing  tileProvider._drawCommands[1].logDepth.receive
Executing  tileProvider._drawCommands[2].logDepth.receive
Executing  tileProvider._drawCommands[3].logDepth.receive
Executing  tileProvider._drawCommands[4].logDepth.receive
Executing  tileProvider._drawCommands[5].logDepth.receive
Executing  tileProvider._drawCommands[6].logDepth.receive
Executing  tileProvider._drawCommands[7].logDepth.receive
Executing  viewportQuadCommand
Executing  primitive.colorCommands[0].logDepth.receive
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[1]-primitive[0].logDepth.receive
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[2]-primitive[0].logDepth.receive
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[3]-primitive[0].logDepth.receive
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[0].logDepth.receive
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[1].logDepth.receive
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[2].logDepth.receive
Executing  viewportQuadCommand
Executing  primitive.colorCommands[0].logDepth.receive.translucent
Executing  primitive.colorCommands[1].logDepth.receive.translucent
Executing  primitive.colorCommands[0].logDepth.receive.translucent
Executing  primitive.colorCommands[1].logDepth.receive.translucent
Executing  viewportQuadCommand
Executing  viewportQuadCommand
Executing  primitive.colorCommands[0].cast
Executing  primitive.colorCommands[1].cast
Executing  primitive.colorCommands[0].cast
Executing  primitive.colorCommands[1].cast
Executing  tileProvider._drawCommands[0].cast
Executing  tileProvider._drawCommands[1].cast
Executing  tileProvider._drawCommands[2].cast
Executing  tileProvider._drawCommands[3].cast
Executing  tileProvider._drawCommands[4].cast
Executing  tileProvider._drawCommands[5].cast
Executing  tileProvider._drawCommands[6].cast
Executing  tileProvider._drawCommands[7].cast
Executing  primitive.colorCommands[0].cast
Executing  primitive.colorCommands[1].cast
Executing  primitive.colorCommands[0].cast
Executing  primitive.colorCommands[1].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[1]-primitive[0].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[2]-primitive[0].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[3]-primitive[0].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[0].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[1].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[2].cast
Executing  tileProvider._drawCommands[0].cast
Executing  tileProvider._drawCommands[1].cast
Executing  tileProvider._drawCommands[2].cast
Executing  tileProvider._drawCommands[3].cast
Executing  tileProvider._drawCommands[4].cast
Executing  tileProvider._drawCommands[5].cast
Executing  tileProvider._drawCommands[6].cast
Executing  tileProvider._drawCommands[7].cast
Executing  primitive.colorCommands[0].cast
Executing  primitive.colorCommands[1].cast
Executing  primitive.colorCommands[0].cast
Executing  primitive.colorCommands[1].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[1]-primitive[0].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[2]-primitive[0].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[3]-primitive[0].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[0].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[1].cast
Executing  model-../../SampleData/models/GroundVehicle/GroundVehicle.glb-node[0]-primitive[2].cast
Executing  tileProvider._drawCommands[0].cast
Executing  tileProvider._drawCommands[1].cast
Executing  tileProvider._drawCommands[2].cast
Executing  tileProvider._drawCommands[3].cast
Executing  tileProvider._drawCommands[4].cast
Executing  tileProvider._drawCommands[5].cast
Executing  tileProvider._drawCommands[6].cast
Executing  tileProvider._drawCommands[7].cast
Executing  tileProvider._drawCommands[0].cast
Executing  tileProvider._drawCommands[1].cast
Executing  tileProvider._drawCommands[2].cast
Executing  tileProvider._drawCommands[3].cast
Executing  tileProvider._drawCommands[4].cast
Executing  tileProvider._drawCommands[5].cast
Executing  tileProvider._drawCommands[6].cast
Executing  tileProvider._drawCommands[7].cast

Now, I don't know for sure what primitive.colorCommands[1].logDepth.receive.translucent is actually bringing on the screen. But for now, I find it at least 'interesting' that this is not the "color command" (or rather one of the two color commands) of a primitive, and not its logDepth, and not its receive, but the translucent derived one of all that.

(Maybe I've let myself be overwhelmed with something that appeared to be pretty complex, but it isn't (insofar that this complexity might not be relevant for the task at hand). But again: I'm not proposing the changes in this PR (and we can probably simply close it, if nobody sees a benefit in that). I'm rather looking for ways to make certain debugging tasks easier...)

@ggetz
Copy link
Contributor

ggetz commented Sep 19, 2024

@javagl What do you think about transferring this to an issue to discuss?

@javagl
Copy link
Contributor Author

javagl commented Sep 20, 2024

Will open an issue soon.

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.

3 participants