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

feat(profiling): add text renderer #31108

Merged
merged 4 commits into from
Jan 21, 2022
Merged

feat(profiling): add text renderer #31108

merged 4 commits into from
Jan 21, 2022

Conversation

JonasBa
Copy link
Member

@JonasBa JonasBa commented Jan 13, 2022

TextRenderer class for rendering text on the canvas.

Since WebGL doesnt have very nice support for text rendering and we dont want to mess around with glyphs, we use an overlay canvas that draws text onto the rectangles.

CleanShot.2022-01-18.at.09.18.39.mp4

@@ -198,13 +198,13 @@ export class Rect {
return this.x;
}
get right(): number {
return this.left + this.width;
return this.x + this.width;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to prefer this.x over this.left? Also if they refer to the same value, can we consolidate this and strictly use this.left and that's more explicit compared to this.x.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right. Left is mostly here because we also have "right", so left is basically a duplicate of x and right is the useful one. I think I originally added it just because it made sense to cover all the coordinates as I added the right and bottom method, but you are right that it creates confusion. I still find it semi-useful in a ways because I would use them differently given the context/operation that I'm doing. E.g. for draw calls, I rather use left/right, as they imply some computed value whereas x, y I would mostly use for more "math" like operations.

}

// @TODO check if we still need this
if (inverted) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does inverted mean here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inverted aka top-down flamegraph

static/app/utils/profiling/renderers/textRenderer.tsx Outdated Show resolved Hide resolved
static/app/utils/profiling/renderers/textRenderer.tsx Outdated Show resolved Hide resolved
static/app/utils/profiling/renderers/textRenderer.tsx Outdated Show resolved Hide resolved

// Check if our rect overlaps with the current viewport and skip it
if (
isOutsideView(frameInConfigSpace, configViewSpace, !!this.flamegraph.inverted)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is because I'm not sure what inverted means, but if inverted is already used to initialize the Rect, it shouldn't be needed for this isOutsideView check right?

Copy link
Member Author

@JonasBa JonasBa Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we dont initialize inverted with Rect right now because I dont want to tie it to the flamegraph. The problem is that currently we kind of fake invert rectangles... we remap their depth, whereas the real solution would be to change coordinate system. That could be a good task if you want to take a stab at it. It would allow us to simplify some of the code and make it more straightforward

@JonasBa JonasBa merged commit cb028e7 into master Jan 21, 2022
@JonasBa JonasBa deleted the jb/profiling/text-renderer branch January 21, 2022 06:14
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants