Skip to content

Change the stack chart drawing strategy for small gaps #1424

Merged
gregtatum merged 6 commits into
firefox-devtools:masterfrom
gregtatum:stack-chart-drawing-strategy
Nov 2, 2018
Merged

Change the stack chart drawing strategy for small gaps #1424
gregtatum merged 6 commits into
firefox-devtools:masterfrom
gregtatum:stack-chart-drawing-strategy

Conversation

@gregtatum

Copy link
Copy Markdown
Member

This PR changes the strategy for drawing the stack chart to draw small
boxes freely, but to leave 1 pixel gaps between boxes. If there are many
small boxes, it skips them until it finds either the next small box 1
pixel over, or the next box that is larger than 1 pixel.

In addition, it provides a small protection against calls to set
fillStyle which are slow due to the re-parsing of the CSS color.

@gregtatum gregtatum requested a review from brisad October 26, 2018 20:39
@codecov

codecov Bot commented Oct 26, 2018

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@c76baa4). Click here to learn what that means.
The diff coverage is 95.94%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1424   +/-   ##
=========================================
  Coverage          ?   80.42%           
=========================================
  Files             ?      156           
  Lines             ?    10586           
  Branches          ?     2582           
=========================================
  Hits              ?     8514           
  Misses            ?     1876           
  Partials          ?      196
Impacted Files Coverage Δ
src/components/flame-graph/Canvas.js 64.02% <ø> (ø)
src/components/marker-chart/Canvas.js 90.32% <ø> (ø)
src/components/stack-chart/Canvas.js 90.44% <100%> (ø)
src/utils/index.js 100% <100%> (ø)
src/components/shared/chart/Canvas.js 86.48% <76.92%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c76baa4...58678ac. Read the comment docs.

@brisad brisad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice, but there was a bit of superfluous code that seems to have sneaked in and should most likely be removed.

The flame graph code should probably have the same changes, as its box-drawing is based on this code. If not addressed here perhaps we should write an issue for that one too?

Comment thread src/components/stack-chart/Canvas.js Outdated
Comment thread src/components/stack-chart/Canvas.js Outdated
Comment thread src/utils/index.js Outdated
@gregtatum gregtatum force-pushed the stack-chart-drawing-strategy branch 3 times, most recently from f3ea735 to 75e64ec Compare October 31, 2018 12:59
@gregtatum

Copy link
Copy Markdown
Member Author

@brisad Could you re-review? After going through this, I ended up changing the behavior to what we discussed on slack with drawing by leaving a slightly transparent pixel gap between concurrent boxes. I broke out the new work into separate commits, but I found reading them all together to still be easier.

Unchanged:
37e6fd9 Change the stack chart drawing strategy for small gaps

Addresses your earlier review:
8de2d15 Remove outdated comment in FastFillStyle

This is a bit of an intermediate step, but it was helpful to get to the final solution:
faab879 Use device pixels in the drawCanvas method of the stack chart

This contains the new strategy, and addresses the clean up of the fill style issues.
bdf9829 Change stack chart drawing strategy to avoid gaps

The final updated snapshot:
75e64ec Update snapshots for the changed rendering strategy

@gregtatum

Copy link
Copy Markdown
Member Author

@brisad brisad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@gregtatum Of course! This looks great! I only have a devicePixelRatio of 1, so I cannot test with anything other than that, but it's real nice that all those small boxes become visible :)

Approved, with just a couple of very minor comments which you perhaps could address.

Comment thread src/components/stack-chart/Canvas.js Outdated
Comment thread src/components/stack-chart/Canvas.js
Comment thread src/components/stack-chart/Canvas.js
Comment thread src/components/stack-chart/Canvas.js Outdated
This PR changes the strategy for drawing the stack chart to draw small
boxes freely, but to leave 1 pixel gaps between boxes. If there are many
small boxes, it skips them until it finds either the next small box 1
pixel over, or the next box that is larger than 1 pixel.

In addition, it provides a small protection against calls to set
fillStyle which are slow due to the re-parsing of the CSS color.
@gregtatum gregtatum force-pushed the stack-chart-drawing-strategy branch from 5266582 to 58678ac Compare November 2, 2018 19:51
@gregtatum gregtatum merged commit b4a107b into firefox-devtools:master Nov 2, 2018
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.

2 participants