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

Implementation of connectgaps for the surface trace #3638

Merged
merged 4 commits into from
Mar 21, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Mar 15, 2019

Fixes #2812.

TODO:

@etpinard
Copy link
Contributor

Looking good @archmoj Some very nice method ♻️ work here!

Bringing in @alexcjohnson here, I remember we had a discussion about surface vs heatmap/contour interpolation with Mikola a few years back (I found https://github.com/plotly/streambed/issues/2268, but maybe it's somewhere else). Does simply reusing the heatmap/ methods for surface feel ok to you?

@alexcjohnson
Copy link
Collaborator

Looks good to me. Whether it's fast... that's another question. It's not slower than heatmap/contour anyway, and it only adds this overhead if you opt in, so I'm happy to add it here. If someone's sufficiently interested in improving performance later we can investigate reimplementing this on the GL side.

I'm more bothered though by what happens with connectgaps turned off for this data - the trace disappears entirely, since there are no mesh squares with all corners specified. Compare that to contour for the same data without connectgaps:
Screen Shot 2019-03-19 at 3 36 40 PM
There we give even islands or thin strips some nonzero size, and three corners of a square are enough to fill in half of the square.

But that's a more complex issue for another time.

@archmoj
Copy link
Contributor Author

archmoj commented Mar 19, 2019

Glad you liked the quick prototype!
We may still need to find a way to fix the hover.
Please try this codepen and possibly describe what should be the expected behaviour if the user hover over the interpolated points?
As a side note for isosurface traces the interpolated data is not displayed.

@alexcjohnson
Copy link
Collaborator

Ah good call on hover, NaN labels squished in the corner aren't too useful 😉
Contour treats the interpolated points no differently from real data. Perhaps at some point it would be nice to indicate which are interpolated, but as a first cut matching contour and labeling exactly what was drawn should be fine.

var v = this.data.z[b][a];

if(v === null && this.data.connectgaps && this.data._interpolatedZ) {
v = this.data._interpolatedZ[b][a];
Copy link
Contributor

@etpinard etpinard Mar 20, 2019

Choose a reason for hiding this comment

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

Looks good. Now that gl3d jasmine tests are behaving better, let's add one 🔒 ing this down!

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 call. Done in afe36a6.

@etpinard etpinard added this to the v1.46.0 milestone Mar 21, 2019
@etpinard
Copy link
Contributor

Nicely done @archmoj

💃 💃

@archmoj archmoj merged commit c73e84b into master Mar 21, 2019
@archmoj archmoj deleted the fix2812-surface-connectgaps branch March 21, 2019 13:43
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