Check axis spec :major / :minor instead of :visible when drawing grid.

[dimovich]

Currently when the viz-spec has :visible set to false, the grid is not drawn even though minor-x or minor-y is set to true.

[postspectacular]

The only potential issue I can see here is that when an axis has its :visible flag disabled, then it's likely it also won't have values for :minor & :major tick resolutions and your new check would then still not show the grid... maybe this is something which should be explained in the docs/comments...

Other than that - thanks! :) 👍

[dimovich]

It makes sense to check for both :visible and :minor-y / :minor-x, because otherwise we won't be able to display major tick grid lines when :minor-x / :minor-y are not defined.

The question is, if :visible is set to false and :grid is {}, should the major tick grid lines still be drawn?

[postspectacular]

To my mind, if :visible is false and no real grid config is given, then yes, major tick lines should still be shown, but in practicae that would only be the case if the axis spec also has :major defined...

[dimovich]

In that case this check should be enough, and these ones could be removed. So as long as :grid is defined and there is either :major or :minor present the grid will be shown.

[postspectacular]

You're right you could leave these out in principle, but would like to keep them for performance reasons / realtime use cases (animated charts), i.e. to avoid the extraneous work & function calls. I understand these won't make much of difference, but every little helps and adds up...

[dimovich]

Because of the way the library works now, it's better to leave it as is.

lin-tick-marks is using rounding, so most of the time the grid lines are different than the axis.

One of our projects needs charts with fixed grid lines, so I removed the rounding, and it generates grid lines that start at the same position as the axis. That's where the need for :visible false came from.

Some middleground I guess would be to change the checks in svg-axis-grid2d-cartesian to:

(if (or (:major x-axis) (:minor x-axis)) ...)
(if (or (:major y-axis) (:minor y-axis)) ...)

Another thing that we came across was when the rounding is removed, sometimes there are charts that have the last grid line out of range by 1e-13, so it is filtered out.

[dimovich]

Maybe it's better to use norm-range instead of range here https://github.com/thi-ng/geom/blob/feature/no-org/src/thi/ng/geom/viz/core.cljc#L347

[postspectacular]

Hey @dimovich - why did you close this PR? i'm sorry I'm been neglecting merging this, since I wanted to do more experiments/tests with these changes before...

[dimovich]

Apologies if that's the case! I was under the impression that due to how the library works now (grid line ticks being rounded) there is no need to add additional checks. All the issues appear when one removes this rounding.