-
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 description to DrawCommand #12119
Conversation
Thank you for the pull request, @javagl! ✅ We can confirm we have a CLA on file for you. |
@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. |
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 |
|
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 For example, looking at the "Shadows" sandcastle with this branch, some log output from a single frame prints
Now, I don't know for sure what (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...) |
@javagl What do you think about transferring this to an issue to discuss? |
Will open an issue soon. |
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 theDrawCommand
. Whenever aDrawCommand
is created, adescription
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" withDrawCommand.shallowClone
, then a suffix is passed in that will be appended to thedescription
.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):
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
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change