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

Introduce max_auto_ticklabel_spacing! for less jitter #4607

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

SimonDanisch
Copy link
Member

@SimonDanisch SimonDanisch commented Nov 20, 2024

To not have jitter in the axis when panning/zooming, we've been recommending to use tight_ticklabel_spacing!, which doesn't fully work to avoid jitter.

  1. It only considers the current ticks, so if the ticks grow, it will not allocate enough tickspace. This is solved by adding n_tick_chars=automatic to tight_xticklabel_spacing! where automatic replicated the old behavior. With set to e.g. 5, it will allocate enough space for 5 chars of ticks, e.g. "0.001".
  2. Zoom and pan use timed_ticklabelspace_reset to reset the tickspace less often when zooming around. But it also resets the tickspace back to automatic (this may be a bug?). In any case, when we want truly static ticks, this should not run at all/ To achieve this, I added a a new option to turn of the timed reset to tight_ticklabel_spacing! and Axis.

Open question: maybe this should be put in a new function, e.g. static_tickspace!(ax, nchars) ?
cc: @jkrumbiegel

@MakieBot
Copy link
Collaborator

MakieBot commented Nov 20, 2024

Benchmark Results

SHA: 250e9e0ec37bf05a4b82adc7c982666cc699bc8a

Warning

These results are subject to substantial noise because GitHub's CI runs on shared machines that are not ideally suited for benchmarking.

GLMakie
CairoMakie
WGLMakie

@jkrumbiegel
Copy link
Member

I'm not sure how much is gained here vs just doing ax.yticklabelspace = some_value? We already have an exact version with tight_ticklabel_spacing! but I'm not sure we need an inexact one. Also the nchars thing doesn't really make sense for both dimensions or if you consider ticklabel rotation.

@SimonDanisch
Copy link
Member Author

SimonDanisch commented Nov 20, 2024

Well, as pointed out, the "exact" version is not really exact once you start zooming.
And I'm not sure how to set the value manually without trial and error, or doing exactly what I'm doing in this PR, which seems a bit too complicated to be left to the user.

Also the nchars thing doesn't really make sense for both dimensions or if you consider ticklabel rotation.

I guess we could use more attributes from the ticklabels, but replacing the text with n_chars?

@jkrumbiegel jkrumbiegel changed the title truely static tickspacing Truly static tickspacing Nov 21, 2024
@jkrumbiegel
Copy link
Member

Well, as pointed out, the "exact" version is not really exact once you start zooming.

I didn't mean that it was useful for zooming specifically, just that it's purpose is an exact one (you wouldn't be able to pick the value it chooses for yticklabelspace easily yourself).

The nchars api feels weird to me because chars can have different sizes so it's not clear what that's supposed to mean, plus as I said, the meaning changes depending on rotation etc.

How about a mode like :max_auto where the tick spacing adjusts, but only upwards. So if you get longer tick labels, there will be room for them, but if they get shorter again, the spacing doesn't change?

@SimonDanisch
Copy link
Member Author

How about a mode like :max_auto where the tick spacing adjusts, but only upwards. So if you get longer tick labels, there will be room for them, but if they get shorter again, the spacing doesn't change?

That's not a bad idea :)

@SimonDanisch
Copy link
Member Author

Ok, I reverted most of my changes to tight_ticklabel_spacing! besides a few cosmetic changes and comments.
I'm pretty confused what tight_ticklabel_spacing! is supposed to do when zooming, especially it's interaction with timed_ticklabelspace_reset. I'm not really sure if timed_ticklabelspace_reset is actually working correctly and it seems out of scope to further debug it here. Maybe @jkrumbiegel can give it a look.
Now, the main change in this PR is to introduce ax.xticklabelspace=:max_auto which will only grow the ticklabelspace, the aforementioned use_zoom_reset_timer/ use_pan_reset_timer and max_auto_ticklabel_spacing!(ax) to apply it to both axes and disable the zoom_reset timer.

@SimonDanisch SimonDanisch changed the title Truly static tickspacing Introduce max_auto_ticklabel_spacing!` for less jitter Nov 29, 2024
@SimonDanisch SimonDanisch changed the title Introduce max_auto_ticklabel_spacing!` for less jitter Introduce max_auto_ticklabel_spacing! for less jitter Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Work in progress
Development

Successfully merging this pull request may close these issues.

3 participants