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

Automatic alignment of ticks in 3D scenes #3131

Merged
merged 6 commits into from
Oct 23, 2018
Merged

Automatic alignment of ticks in 3D scenes #3131

merged 6 commits into from
Oct 23, 2018

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 20, 2018

This PR improves automatic alignment of ticks on 3D axes. The best single orientation for each axis (depending on the camera position and orientation) is found and used by vertex shader.
In the earlier work it was possible for the ticks on an axis to align themselves in two different directions and stay upwards. But such behavior was not completely desirable in some perspective views.
@alexcjohnson

package.json Outdated
@@ -78,7 +78,7 @@
"gl-mat4": "^1.2.0",
"gl-mesh3d": "^2.0.0",
"gl-plot2d": "^1.3.1",
"gl-plot3d": "^1.5.7",
"gl-plot3d": "git://github.com/gl-vis/gl-plot3d.git#240870c6cbf602b956363b86287e516a45969ea5",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson
Copy link
Collaborator

@archmoj interesting - The code looks great and it does exactly what we discussed, can't argue with any of that! But now that I see it and play with it, I'm actually not convinced that this is better than having each label independently restricted to [-90,90] degrees rotation as implemented in #3084. I kind of think the improved consistency from label to label is not worth the reduced legibility of individual labels. What do you think?

@etpinard @antoinerg thoughts?

@antoinerg
Copy link
Contributor

such behavior was not completely desirable in some perspective views.
Hello @archmoj, can you point me to some examples of those perspective views where the results are undesirable.

I kind of think the improved consistency from label to label is not worth the reduced legibility of individual labels.
@alexcjohnson I do agree that the labels oriented with an angle outside [-90, 90] look weird to me and are probably a bit harder to read. On the other hand, I am not sure I fully appreciate what we're gaining in consistency. I need to see an example and test it out more.

@alexcjohnson
Copy link
Collaborator

@antoinerg when @archmoj first implemented the rotated labels I said I wished the labels for one axis would all get the same rotation, so you could read them as a series. That’s the consistency argument. Try it on master, as you slowly rotate a scene you’ll see the labels flip one at a time as each grid line passes through vertical. In this PR they all flip together.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 20, 2018

such behavior was not completely desirable in some perspective views.
can you point me to some examples of those perspective views where the results are undesirable.
@antoinerg These old and new files may be easy to compare using Swipe option.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 20, 2018

What do you think?

We could setup the shader to be able to serve both functionalities (as well as others e.g. option:2 for horizontal/vertical, option:8 for 8 directions ...). For example I could simply allocate option:0 (now used for debugging) for the ranges between [-90,90]. Then the question would be which option 0/1 should be the default behavior. I personally think that when either the ticks include some text or the user need to print the graph it would be nice to give them options to switch to this new behavior.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 20, 2018

@antoinerg
Here is an example of the previous behavior:
created by @alexcjohnson before.

label flip

@archmoj archmoj added the feature something new label Oct 21, 2018
@antoinerg
Copy link
Contributor

antoinerg commented Oct 22, 2018

This is maybe not the right place to bring this up but why aren't we making the label facing us and always horizontal?

For example, why aren't the Moneyness axis labels simply horizontal:
newplot

A lot of 3D scatters are done that way. For example:
matlab_scatter_3d

This is the easiest thing to read IMHO. I most certainly don't appreciate the different edge cases and shortcoming of this approach.

@alexcjohnson
Copy link
Collaborator

This is maybe not the right place to bring this up but why aren't we making the label facing us and always horizontal?

Absolutely, a reasonable place to bring it up. I can certainly see cases where horizontal is better, and a lot of simple situations fall into this camp. But there are also cases - particularly when there are lots of ticks (so it can be ambiguous which label goes with which tick) or long labels (so they can overlap each other) - where to my eye it's much better to rotate the labels. So if we only have one behavior I think it should be this one (specifically restricting each label independently to [-90, 90] degrees) but I'd be happy for us to enable fixed angle (normally fixed at 0) and consistently rotated angles in a subsequent PR.

package.json Outdated
@@ -78,7 +78,7 @@
"gl-mat4": "^1.2.0",
"gl-mesh3d": "^2.0.0",
"gl-plot2d": "^1.3.1",
"gl-plot3d": "^1.5.7",
"gl-plot3d": "git://github.com/gl-vis/gl-plot3d.git#ecb766038c6c3bb041a6405795c42512c56f5387",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually pointing to this change on gl-axes3d:
gl-vis/gl-axes3d@master...align-ticks

@alexcjohnson
Copy link
Collaborator

OK great - I'm glad this now leaves all the mocks as they were before the PR. Since this doesn't have any immediate effect anymore, let's wait and see if @etpinard's opinion changes the result at all.

@alexcjohnson
Copy link
Collaborator

Oh actually, I forgot, this PR includes the Chrome 51 title rotation fix, right? In that case, lets go ahead and merge the upstream PRs and this, and we can discuss whether to change the default behavior and/or add another config parameter in a separate PR. 💃

@archmoj archmoj merged commit 0b99103 into master Oct 23, 2018
@etpinard etpinard added this to the v1.42.0 milestone Oct 23, 2018
@etpinard
Copy link
Contributor

etpinard commented Oct 23, 2018

I'm glad this now leaves all the mocks as they were before the PR.

I'm very glad of that as well. 👍

@etpinard's opinion changes the result at all.

I much prefer the behavior now. 🥇


Pro tip for @archmoj: you should delete your PR branches after merging them (I've done so here, no worries), to reduce the chance of collisions between local and origin branch names.

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

Successfully merging this pull request may close these issues.

None yet

4 participants