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

Rotate axis labels that are longer than 4 characters in 3D scenes (issue #3077) #3084

Merged
merged 28 commits into from
Oct 12, 2018

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 7, 2018

This PR is to enable the interactive alignment of axis tiles (labels) that are long i.e. to avoid the overlap of layout descriptions with data representation: issues #3077 and #511 .
For various reasons this functionality is not applied for labels that are shorter than 4 characters e.g. 'x', 'y', 'z'. As a result during the image test there were only 2 images that needed to be replaced which now have vertical titles on their z-axis.
To achieve this, changes are made to textVert.glsl shader as well as axes.js of gl-axes3d module.
Having this new setup, it could also be possible to orient axis ticks as well (once there is a config option).
It may also be possible to remove horizontal & vertical alignments locks in the shader and to try orienting every title towards its axis.
@etpinard
@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.5",
"gl-plot3d": "git://github.com/archmoj/gl-plot3d.git#316309c4ede55f990a61df5ba91f0f52bad07374",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.5",
"gl-plot3d": "git://github.com/archmoj/gl-plot3d.git#c173daf6ed8bb74b5db9feedee9f66b4e39a638d",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard
Copy link
Contributor

etpinard commented Oct 9, 2018

Let's try to get this part of v1.42.0

@etpinard etpinard added this to the v1.42.0 milestone Oct 9, 2018
@etpinard
Copy link
Contributor

etpinard commented Oct 9, 2018

Here's a look at @archmoj's "dynamic" behavior:

peek 2018-10-09 12-24

@alexcjohnson
Copy link
Collaborator

FYI @archmoj and I agreed that he will extend this so the titles exactly match the axis angles, rather than being restricted to exactly horizontal or vertical.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 10, 2018

Alex, what do you think about these?
newplot 40
newplot 39
newplot 41

@archmoj
Copy link
Contributor Author

archmoj commented Oct 12, 2018

@alexcjohnson This could be a good version to test & merge.
@etpinard Could you please help with updating the baseline images? This time the image test on CircleCi stopped after encountering only a few differences. So I was not able to collect the rest of changes. Thank you!

@archmoj
Copy link
Contributor Author

archmoj commented Oct 12, 2018

No need for generating baseline images. They are uploaded. I am looking forward for your review.
@etpinard @alexcjohnson

@alexcjohnson
Copy link
Collaborator

Whew, late night @archmoj! 😴

Tick labels on all 3 axes behave the same now, which is great 🎉 and the titles are still fantastic 🏆
But the condition for flipping the tick labels between rotated left and rotated right seems weird. Particularly if I point one of the axes close to straight in/out of the screen, I can get labels rotated far past vertical. They're still all determining when to flip independently, and again I wouldn't spend time on that issue right now, but in lieu of a solution like that (which, based on some experimentation like the bottom image below I'm not even sure is a good idea anymore) we should simply restrict their individual angles to [-90, 90]. Some examples of what I see now:

All "moneyness" tick labels are upside-down:
screen shot 2018-10-12 at 9 24 47 am

140 and 160 are past vertical but at least consistent with 40-120, yet 180 and 200 flip to right-side-up:
screen shot 2018-10-12 at 9 31 21 am

@archmoj
Copy link
Contributor Author

archmoj commented Oct 12, 2018

Thanks for your observations. I agree for ticks we could also the set the default tolerance to zero.
On another topic, what do you think about the maximum number of characters in titles so that stay horizontal? I was wondering it may be better to use a number less than 4?

@alexcjohnson
Copy link
Collaborator

I was wondering it may be better to use a number less than 4?

I don't have a strong opinion about this. 1-letter titles should definitely NOT be rotated, as then you don't have enough context to figure out how to read it. Long titles must be rotated (that's how we got into this issue after all!) But I could see an argument for anywhere from 2 to 4 letters being the longest unrotated title.

At some point we will make this configurable, for now lets leave it at 4 unless anyone else has strong feelings otherwise.

@alexcjohnson
Copy link
Collaborator

Much better. I can still find occasional cases where it doesn't behave quite as I would like, but I think we should merge this and move on. Thanks @archmoj! Can you work with @etpinard to get these changes published in the gl-* packages and updated back to npm references? Then 💃 !!!

@etpinard
Copy link
Contributor

Hmm @archmoj

The gl3d_surface-lighting mock should not have changed:

peek 2018-10-12 14-33

as it has

image

@archmoj
Copy link
Contributor Author

archmoj commented Oct 12, 2018

I think the image above compares the deleted version with the new version. But if we compare the master version with the new image, there is actually no difference. And the angles are explicitly defined for each axes as expected.

@archmoj archmoj merged commit 68bd3f7 into master Oct 12, 2018
@archmoj archmoj deleted the rotate3d-axis-lables branch October 12, 2018 21:10
@antoinerg
Copy link
Contributor

Congratulations!

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.

4 participants