Bug in calculateTickRotation for logarithmic scale

[benmccann]

calculateTickRotation calls getPixelForTick(0), which depends on _valueOffset. _valueOffset depends on _length and is set in _configure which is called before fit. However, _length is not properly initialized until fit is called

I'm not exactly sure what the point of _valueOffset is actually. It seems buggy in more ways than one. Why is it dividing the font size by the length? Is this logic correct for both horizontal and vertical scales? Should it take the label rotation into account?

There's sort of a nasty recursive logic here where the axis length depends on the tick rotation and the tick rotation depends on the axis length. And also buildTicks depends on tick capacity depends on axis length depends on tick rotation depends on the number of ticks. This circular logic causes lots of issues, code duplication, and overall makes the logic very hard to understand. For example, see also #6791 (comment). I'd love any ideas on how we break this circular logic. As a chart.js dev, this has probably caused me more frustration than anything else in the codebase

What if we removed calculateTickRotation and the minRotation / maxRotation options and instead had a labelRotation option that was scriptable? That would both drastically simplify our code and give the user more control over the label rotation. E.g. right now the code assumes the user wants 3 pixels of padding, which isn't configurable

[kurkle]

I was thinking the tick rotation calculation could be simplified and require user supplied number of ticks.
So I'd remove the min/max rotation options, replace with just rotation and allow 'auto' for this simplified version.

[kurkle]

Thinking more, the number of ticks needs to be determined first, without considering labels. This would actually solve the problem with time scale generate/autoSkip.

[kurkle]

And detecting the loop in my thought process, in first _configure the number of ticks could be always 2?

[benmccann]

There's a couple more things I figured out. E.g. even if we know the rotation value upfront there's still a dependency between buildTicks and axis length unless we break it another way (because buildTicks tries to estimate how many ticks can fit and the axis length is adjusted in fit based on the label sizes)

I was thinking the tick rotation calculation could be simplified and require user supplied number of ticks.

Making the user supply the number of ticks might not be feasible for auto generated ticks. One thing I figured out is that we can actually know a max number of ticks ahead of time though even for auto generated ticks and could probably add another method to return that (though it's complicated for the log scale). However, the tick rotation code also currently needs the labels, so it's sort of hard to decouple from buildTicks too much

The number of ticks needs to be determined first, without considering labels. This would actually solve the problem with time scale generate/autoSkip.

If we generate a fair number of extra ticks I think that's probably fine (sort of how it works now in master). What can get a little weird is if we try to calculate a number of ticks too exactly. The issue is that after buildTicks is called we call fit, which updates _length based on the labels and tick rotation and so the axis shrinks. Then our calculation was off and we have to skip ticks anyway. There's all this code and complication in the time scale based on trying to estimate the number of ticks, but it's always wrong since we don't know _length. If we can just give up on trying to be too exact then I think it's fine. But then there's the question of do we still have buildTicks return objects? If so then I think that gets pretty expensive to have extra ticks returned from buildTicks

in first _configure the number of ticks could be always 2

I'm not sure I understand this suggestion.

[kurkle]

I'm not sure I understand this suggestion.

Sorry, I did not validate my thoughts with the code.
Currently, _configure is called after ticks are build, so it does not make any sense.

I was thinking the first update, that is called to determine required padding, and for that, we only need the ticks that are affecting the size of the scale (=min/max tick)

[kurkle]

Does not change the problem, but maybe adding a determinePadding that could be called multiple times and call update only when dimensions are final, would make things easier.

[benmccann]

_valueOffset was removed