-
Notifications
You must be signed in to change notification settings - Fork 64
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
Don't merge scopes that have different data #102
Conversation
Previously we would merge all scopes that have the same `id`, but if you have multiple scopes after each that have same `id` but different dynamic `data` field they would be merged together and we wouldn't display the `data` field. This is something we ran into in our codebase that made it harder to see why a certain heavy operation was happening as we were missing the context for it. Which we did specify but was merged together and not shown here as merging scopes is on by default (and is a general good idea). Adds a simple visual test for in the imgui and e gui clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code makes sense 👍 Thanks for fixing that!
removing @VZout as @TimonPost is going to take over as CODEOWNER on the Embark side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Maybe add a line in the appropriate changelogs? |
@TimonPost could do a new |
Previously we would merge all scopes that have the same
id
, but if you have multiple scopes after each that have sameid
but different dynamicdata
field they would be merged together and we wouldn't display thedata
field.This is something we ran into in our codebase that made it harder to see why a certain heavy operation was happening as we were missing the context for it. Which we did specify but was merged together and not shown here as merging scopes is on by default (and is a general good idea).
Adds a simple visual test for in the imgui and egui clients.
Resolves: #73
Example
This is how it looks like now:
from the following code:
Before this PR this would have been a single "4x Spike"