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

Add autotickangles property #6790

Merged
merged 36 commits into from
Jan 26, 2024
Merged

Add autotickangles property #6790

merged 36 commits into from
Jan 26, 2024

Conversation

my-tien
Copy link
Contributor

@my-tien my-tien commented Nov 22, 2023

The new autotickangles property can be used to specify a choice of rotation angles for the labels on the x-axis.
If tickangle is set to "auto", it will be set to the first angle in this array that is large enough to prevent label overlap.

Previously, when tickangle was set to "auto", plotly would choose between a hardcoded 30 or 90 degrees which is still the default for autotickangles, so backwards-compatibility is maintained.

… 90 or -90 degrees.

Previously this check only tested 90 degrees.
when tickangles is set to "auto", this array will be searched for the first angle large enogh to prevent overlap between labels
@my-tien my-tien marked this pull request as ready for review November 22, 2023 14:27
@@ -3806,8 +3820,8 @@ axes.drawLabels = function(gd, ax, opts) {
// N.B. during auto-margin redraws, if the axis fixed its label overlaps
// by rotating 90 degrees, do not attempt to re-fix its label overlaps
// as this can lead to infinite redraw loops!
if(ax.automargin && fullLayout._redrawFromAutoMarginCount && prevAngle === 90) {
autoangle = 90;
if(ax.automargin && fullLayout._redrawFromAutoMarginCount && Math.abs(prevAngle) === 90) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

=== 90 doesn't seem like the right condition here anymore. In fact I'm suspicious of this condition (even before this PR, why is it only a 90-degree rotation that can lead to infinite loops? why not a 30-degree rotation?) so I feel like we should get rid of it and just (inside fixLabelOverlaps) require that if there's a prevAngle we only choose one with Math.abs(newAngle) >= Math.abs(prevAngle)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to insert my change without having to understand the whole file. So I am not sure what exactly would cause an infinite redraw loop. If I remove the entire if-condition (so the code would always do seq.push(fixLabelOverlaps)) and set the angle to >= prevAngle inside fixLabelOverlaps, this will be okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check my latest commit for this change

@archmoj archmoj added feature something new community community contribution labels Nov 23, 2023
@archmoj
Copy link
Contributor

archmoj commented Nov 23, 2023

Interesting PR.
In regards with test failures, please note that several subplots(e.g. polar, gl3d) as well as components (e.g. colorbar) reuse attributes and functions from cartesian. This means when we add a new attribute like this in cartesian we should skip it in other places.
For example if you search for noTicklabelstep in the code you could find places that we didn't want to have ticklabelstep attribute.
Similarly for your new attribute we should pass something noAutotickangles: true and use it in the coerce functions.

@my-tien
Copy link
Contributor Author

my-tien commented Nov 23, 2023

    autotickangles: {
        valType: 'data_array',
        coerceNumber: true,
        dflt: [30, 90],
        editType: 'ticks',
        description: [
            'When `tickangle` is set to *auto*, it will be set to the first',
            'angle in this array that is large enough to prevent label',
            'overlap.'
        ].join(' ')
    },

Could you help me out with the correct definition here (omitting coerceNumber breaks other things)? Because the jasmin test complains:

all attributes should only have compatible options
plot schema
Error: Expected false to be true, 'coerceNumber', Object({ valType: 'data_array', coerceNumber: true, dflt: [ 30, 90 ], editType: 'ticks', description: 'When tickangle is set to auto, it will be set to the first angle in this array that is large enough to prevent label overlap.' }).

@archmoj
Copy link
Contributor

archmoj commented Nov 23, 2023

    autotickangles: {
        valType: 'data_array',
        coerceNumber: true,
        dflt: [30, 90],
        editType: 'ticks',
        description: [
            'When `tickangle` is set to *auto*, it will be set to the first',
            'angle in this array that is large enough to prevent label',
            'overlap.'
        ].join(' ')
    },

Could you help me out with the correct definition here (omitting coerceNumber breaks other things)? Because the jasmin test complains:

all attributes should only have compatible options
plot schema
Error: Expected false to be true, 'coerceNumber', Object({ valType: 'data_array', coerceNumber: true, dflt: [ 30, 90 ], editType: 'ticks', description: 'When tickangle is set to auto, it will be set to the first angle in this array that is large enough to prevent label overlap.' }).

Let's not set coerceNumber: true, for the new attribute as it is not relevant to data_array type.
Instead you should convert string values to numbers in src/plots/cartesian/axes.js.

@my-tien
Copy link
Contributor Author

my-tien commented Nov 24, 2023

Not sure if the last failing test is related to my changes? (test-baselines compare-pixels)

@archmoj
Copy link
Contributor

archmoj commented Nov 24, 2023

Not sure if the last failing test is related to my changes? (test-baselines compare-pixels)

Yes. It is the side effect of your PR. Please see the diffs in the ARTIFACTS tab at https://app.circleci.com/pipelines/github/plotly/plotly.js/9395/workflows/fd12b65d-7c84-4420-8194-2568e9111bc3/jobs/205180/artifacts

Previously, although I used [30, 90] as default autotickangles, this did not reproduce old images, since the condition for choosing between the two was also different.
@my-tien
Copy link
Contributor Author

my-tien commented Nov 24, 2023

Not sure if the last failing test is related to my changes? (test-baselines compare-pixels)

Yes. It is the side effect of your PR. Please see the diffs in the ARTIFACTS tab at https://app.circleci.com/pipelines/github/plotly/plotly.js/9395/workflows/fd12b65d-7c84-4420-8194-2568e9111bc3/jobs/205180/artifacts

Thank you, I introduced the old condition for choosing between 30 and 90 degrees if autotickangles is not provided.

Also, I notice that make-baselines did not regenerate the 10.png, the one I edited to test autotickangles. How do I trigger that?

@archmoj
Copy link
Contributor

archmoj commented Nov 24, 2023

Please download new 10.png baseline from https://app.circleci.com/pipelines/github/plotly/plotly.js/9397/workflows/b958b58d-8d09-4c82-90b8-d49dd68a1524/jobs/205218/artifacts and place it at test/image/baselines/ then commit the change.
Thank you!

if(angle === undefined) {
// no angle larger than minAngle, just pick the largest angle
angle = autoTickAngles.reduce(
function(currentMax, nextAngle) { return Math.abs(currentMax) < Math.abs(nextAngle) ? nextAngle : currentMax; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using an angle with "maximum" abs(t) value, shouldn't we pick the angle with "minimum" abs(cos(t))?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, I hadn't thought about angles beyond [-90, 90]. Would be kind of weird to auto-rotate past vertical but I guess no reason to prohibit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, have changed it to test for minimum absolute cosine.

@alexcjohnson
Copy link
Collaborator

I pushed a small commit that makes the behavior what I wanted: when using autotickangles, start with the first angle in the array even if it isn't 0. I also moved a little more of the logic into the defaults step, so that we don't have autotickangles unless we're going to use it (except for the funny log exclusion), which is more in line with our standard practice so that _fullLayout doesn't contain unused attributes.

Then a couple more to fix colorbar ticks and polar ticks (which both thought they were preventing autotickangles, but they weren't and in certain cases they shouldn't!)

@archmoj @my-tien there's still one failing image that I don't quite understand, I can take a look again in the morning, and for some reason I wasn't able to rebuild the schema tonight. Also the same failing jasmine tests as before. But otherwise I think this is good to go from my side!

@archmoj
Copy link
Contributor

archmoj commented Jan 26, 2024

Thanks very much @alexcjohnson and @my-tien.
After @alexcjohnson improvements, now on the polar_polygon-grids the tick labels on the magenta graph are rotated 90 degrees!

draftlogs/6790_add.md Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Jan 26, 2024

Nicely completed.
[ 💃, 👯 , 🕺 ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants