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

More gl2d symbols! #1781

Merged
merged 19 commits into from
Jun 14, 2017
Merged

More gl2d symbols! #1781

merged 19 commits into from
Jun 14, 2017

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Jun 12, 2017

Supersedes #1550 with better mocks and fine tuning via gl-vis/gl-scatter2d-sdf#4 and b1bccdc

TODO:

David Estrada and others added 10 commits June 12, 2017 13:55
- showing off ALL available symbols!
- so that gl3d list remains the same until
  we spend more time tweaking each symbol's positioning in gl3d
- add per-symbol specs (w/ 'noBorder' and 'bwFactor')
- color `-open` symbols using `marker.color`
- color symbols with no border (e.g. 'line-ne', 'asterisk')
  using `marker.line.color` only
- puts us close to on-par with SVG
- so that it match the svg version
- see https://codepen.io/etpinard/pen/WOxxaO for reproducible codepen
@@ -0,0 +1,505 @@
{
"data": [
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 Author

SVG:

marker_symbols

gl2d:

gl2d_marker_symbols

"marker symbol: 'diamond-open'",
"marker symbol: 'cross'"
],
"marker": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should make sure new marker symbols are positioned and scaled properly in gl3d before adding them. To be completed in a future PR.

@dy
Copy link
Contributor

dy commented Jun 12, 2017

@etpinard if marker set is going to be limited, a better solution would be pre-generated sdf sprite with all markers included. That would enable all svg markers in scattergl and allow to get rid of font-atlas-sdf, which would make code of gl-scatter2d way simpler and performant.
Probably it should be addressed in #949.

@alexcjohnson
Copy link
Collaborator

Could we make some of the markers be rotated characters? So like ➕ could rotate 45 degrees to make ✖️ rather than having to use letter X.

@dy
Copy link
Contributor

dy commented Jun 13, 2017

@alexcjohnson we use font-atlas-sdf to render symbols, which in turn uses tiny-sdf internally to draw characters with API call draw(letter). It is possible, but would unnecessarily bloat API. In fact it is not the X letter used, but valid unicode box cross symbol ╳, which is rendered so with default ubuntu sans-serif font. We can replace it with any other cross.

@alexcjohnson
Copy link
Collaborator

OK - just trying to get closer to parity. Lots of good additions here, but I don't think we should include symbols that don't match fairly closely what we do in SVG. To my eye the red ones below are not close enough as shown, and the green ones could probably be used unfilled but not filled.
gl2d-markers

@etpinard
Copy link
Contributor Author

@alexcjohnson

To my eye the red ones below are not close enough as shown, and the green ones could probably be used unfilled but not filled.

Done in ➡️ bb9a46c

Thanks for taking a look 🔬

@dy
Copy link
Contributor

dy commented Jun 13, 2017

@alexcjohnson thanks for the good point! Fixed in dy/font-atlas-sdf@1f610fe

@etpinard
Copy link
Contributor Author

it's a bit of a shame not to have any "X" symbols...

Yeah, it's a shame. Actually, I should put it back in even it doesn't render in imagetest.

- so that -open symbols are noticeabily NOT filled in white
- used in 'fancy' scattergl traces
- control its version here, instead of it being a 3rd-party dep
- fill 'cross-open' and 'x-open' with transparent
  so that they match the svg version
- N.B. 'x' does not render in `imagetest` (appears as [])
  but appears to work ok in other browsers/OS
- dynamically list -open symbols
- draw -open symbol as filled unicode symbols with transparent fill
  so that the border width scales the same as their non-open versions
  -> this adds support for heaxgon2-open 🎉
- add annotation about 'x' and 'x-open' in `imagetest`
} else if(symbolSpec.noBorder) {
bwFactor = 0.25;
} else if(symbolSpec.noFill) {
bwFactor = 0.1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

so... this means that for these symbols with a smaller bwFactor the border will grow more slowly than the others? The result being that for small borderWidth these ones be thicker than other symbols, and for large borderWidth they will be thinner, and somewhere in the middle they will look similar right? I would think we'd do better with something like a minimum border width, as a fraction of size, that corresponds to the line width already baked into the unicode symbol, so the symbol gets no explicit border until you provide a width greater than this, and at greater widths you set a border borderWidth - minBorderWidth.

Copy link
Contributor Author

@etpinard etpinard Jun 14, 2017

Choose a reason for hiding this comment

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

. this means that for these symbols with a smaller bwFactor the border will grow more slowly than the others?

You're correct. Thanks for the headsup! For documentation purposes, using https://codepen.io/etpinard/pen/JJRMxo?editors=0010 as of commit fffc702

SVG gives:

image

and gl:

image

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
Collaborator

Choose a reason for hiding this comment

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

//this.scatter.options.borderWidths[i] = (bw > minBorderWidth) ? bw - minBorderWidth : 0;

// but I found better results with:
this.scatter.options.borderWidths[i] = (bw > minBorderWidth) ? bw - minBorderWidth : bw;

minBorderWidth should only be nonzero for noBorder or noFill symbols right? Does that make it work better to use bw - minBorderWidth : 0? As it is it looks funny, as border width is non-monotonic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that's better:

3f664a1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome - I think we've got it 🎉 💃

@alexcjohnson
Copy link
Collaborator

@etpinard nice work! just the one more question about how we calculate border widths https://github.com/plotly/plotly.js/pull/1781/files#r121820798 and we should be ready to go. The available symbols are looking really good now. Thanks for bearing with all my nitpicks, I just want to get this all as final as possible so we don't need to come back to it again...

etpinard added a commit that referenced this pull request Jun 14, 2017
@alexcjohnson
Copy link
Collaborator

💃 💃 💃 !

@etpinard etpinard merged commit 91c0b8d into master Jun 14, 2017
@etpinard etpinard deleted the gl2d-more-symbols branch June 14, 2017 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants