Fix issues in log_diagnostics example #18652
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kristoff3r
added
C-Examples
kristoff3r
force-pushed
the
log-diagnostics-example
branch
from
7857795 to
038fb7b
Compare
|
Hmm it looks like there should be render diagnostics, I'll try to figure out why it doesn't print anything and add it back. |
alice-i-cecile
approved these changes
Mar 31, 2025
|
I hate commented out code T_T Thanks for fixing this. |
IceSentry
suggested changes
Apr 1, 2025
I re-added it and modified the example so it actually prints something. Without anything drawn it did nothing which just made it's inclusion in the example confusing. |
shwwwa
approved these changes
Apr 2, 2025
alice-i-cecile
added
S-Ready-For-Final-Review
mockersf
pushed a commit
that referenced
this pull request
Apr 3, 2025
# Objective Adopts / builds on top of #18435. The `log_diagnostics` example has bit-rotted and accumulated multiple issues: - It didn't explain the ordering constraint on DefaultPlugins, as noted by the original PR - Apparently `AssetCountDiagnosticsPlugin` no longer exists (?!). I couldn't figure out when or why it was removed, maybe it got missed in Assets v2? - The comments didn't explain what kind of info you get by the various plugins, making you do work to figure it out - ~As far as I can tell `RenderDiagnosticsPlugin` currently doesn't register any diagnostics in the traditional sense, but is only focused on rendering spans? At least it doesn't print anything extra when added for me, so having it here is misleading.~ It didn't print anything because there was nothing to render in this example ## Solution - Make all plugins be commented in to prevent further bit-rot - Remove reference to the missing plugin - Add extra comments describing the diagnostics in more detail - Add something to render so we get render diagnostics ## Testing Run the example, see relevant diagnostics printed out
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Adopts / builds on top of #18435.
The
log_diagnosticsexample has bit-rotted and accumulated multiple issues:AssetCountDiagnosticsPluginno longer exists (?!). I couldn't figure out when or why it was removed, maybe it got missed in Assets v2?As far as I can tellIt didn't print anything because there was nothing to render in this exampleRenderDiagnosticsPlugincurrently doesn't register any diagnostics in the traditional sense, but is only focused on rendering spans? At least it doesn't print anything extra when added for me, so having it here is misleading.Solution
Testing
Run the example, see relevant diagnostics printed out