-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Fix tick label rotation and layout issues #5961
Conversation
8e6bce3
to
eff623b
Compare
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.
I think I understand how this works. I haven't had a chance to test it out locally yet.
Small performance increase in this PR, rendering 1M points on timescale. Master: {
"1 init": 1.8,
"2 update/ds/0": 782.6,
"2 update/ds/1": 1079.2,
"2 update/ds/2": 748.4,
"2 update/ds/3": 727.7,
"2 update/ds/total": 3338.3,
"2 update/layout": 4393.1,
"2 update/total": 8190.5,
"3 render/draw/ds/0": 105.6,
"3 render/draw/ds/1": 111.3,
"3 render/draw/ds/2": 119,
"3 render/draw/ds/3": 119.9,
"3 render/draw/ds/total": 456.2,
"3 render/draw/total": 458.1,
"3 render/total": 476.5,
"x from page load": 8673.9
} This PR: {
"1 init": 1.7,
"2 update/ds/0": 822.4,
"2 update/ds/1": 824,
"2 update/ds/2": 1046.8,
"2 update/ds/3": 794.2,
"2 update/ds/total": 3488,
"2 update/layout": 2910.7,
"2 update/total": 6803.2,
"3 render/draw/ds/0": 107,
"3 render/draw/ds/1": 108.6,
"3 render/draw/ds/2": 118,
"3 render/draw/ds/3": 123.2,
"3 render/draw/ds/total": 457,
"3 render/draw/total": 459.1,
"3 render/total": 477.9,
"x from page load": 7288.3
} 250k ponts per dataset. something breaks in chrome (render process dies) when putting 300k points in a set. |
@kurkle Thanks for the analysis. It seems that the performance gain comes from "2 update/layout", so this is what I expected. Does the master even crash in Chrome? |
@nagix both crash, so not an issue in this PR. I think maybe path becomes too long. |
b42285d
to
21a6bb6
Compare
There are a few problems
ticks.minor
andticks.major
configuration:scale.ticks.*
options at runtime because the scale class copies the minor and major ticks options fromticks.*
on the initial chart creation, and it doesn’t update them on the following update calls as the values already exist ([BUG] Can't update scale.ticks.* options at runtime since 2.7.0 #4896).ticks.major.enabled
is set tofalse
by default, butticks.major
options are effective. The behavior should be consistent with theticks.major.enabled
setting.ticks.minor
options are used forcalculateTickRotation
,fit
,draw
and_autoSkip
calculations, so major tick labels can overlap or be cropped.calculateTickRotation
only considers the first tick interval before fitting, and doesn’t take into account margins for the tick labels on the left and right.This PR fixes these problems as follows.
mergeTicksOptions
and addsparseTickFontOptions
to parse minor and major tick font options. Omitted options are inherited fromticks
at runtime.ticks.major.enabled
isfalse
.computeTextSize
and addscomputeLabelSizes
to measure tick labels and find the widest, highest, first and last labels at a time.calculateTickRotation
so that it takes into account margins for the tick labels on the left and right.Edit: This PR fixes the following problems as well.
calculateTickRotation
calculates the maximum scale height using the scale label size, the tick mark size and the tick padding to remove the overlap with scale label ([BUG] Tick label overlaps axis label, long labels cropped #5906).calculateTickRotation
has a loop to determine the label rotation and it can be repeated 90 times at most. The proposed version eliminate the loop to improve performance.Edit2: Problem 1 and 2 are addressed in #6102.
Master: https://jsfiddle.net/nagix/amtLsdfb/
This PR: https://jsfiddle.net/nagix/mas98q17/
Fixes #5906
Fixes #2106