Skip to content

FI-51 contour plot fill #522

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

Merged
merged 6 commits into from
May 17, 2016
Merged

Conversation

monfera
Copy link
Contributor

@monfera monfera commented May 10, 2016

Changes:

Related to contourgl fill:

  • changes to gl-vis/gl-contour2d
    • triangle fix from @mikolalysenko
    • use fill color
    • include packages
    • changed example to more fully exercise the code
  • changes to plotly.js
    • use gl-contour2d 1.1.2
    • changes to convert.js for the color fill setup
    • jasmine test cases (they only expect fill contours to work without raising an error)

Related to preparation for contourgl fill work:

  • adding test utils for setup / teardown (removed for the time being, in favor of a TBD solution)
  • refactoring of gl_plot_interact_basic_test using Plotly.purge (residual todo item from the already merged FI-30)

Planned or possible follow-up PRs once the gl-contour2d tessellation issue is solved (post the current v1.1.2):

  • bump gl-contour2d version in package.json
  • image based test cases for contourgl fill / line (now it would 'lock in' problematic artifacts)
  • use of more standard surface test patterns (2D wave interference, saddle, noise etc.)
  • improving similarity of appearance with the non-GL contour e.g. line thickness/color
  • consider oversampling for a much finer grid, because gl-contour2d doesn't handle curves/splines, and the SVG code isn't directly reusable because the SVG implementation properly relies on splines achievable with the path mini-language of the d attribute
  • depending on the solution to the tessellation issue as well as the possible oversampling above, consider refactoring opportunity for sharing more code between GL convert and the non-GL contour

@monfera monfera changed the title WORK IN PROGRESS FI-51 contour plot fill WIP FI-51 contour plot fill May 10, 2016
@monfera
Copy link
Contributor Author

monfera commented May 10, 2016

image

@monfera monfera force-pushed the fi-51-contour-plot-fill branch from a8b2a13 to db69c88 Compare May 13, 2016 14:14
@monfera monfera force-pushed the fi-51-contour-plot-fill branch from db69c88 to 9d7bbfb Compare May 13, 2016 14:18
@monfera monfera changed the title WIP FI-51 contour plot fill FI-51 contour plot fill May 17, 2016

// pass on fill information
if(fullTrace.contours.coloring === 'fill') {
colorOptions = convertColorscale(fullTrace, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with the boolean fill option here, as convertColorscale is only used in this file. But @monfera, I hope you agree that convertColorscale(fullTrace, { fill: true }); would have been cleaner and more robust.

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 Glad to make this small change!

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for now. 👍

@etpinard
Copy link
Contributor

Great. This is a solid patch for contourgl. Thank you very much for your efforts @monfera 🍻

@etpinard etpinard merged commit c76cdf8 into plotly:master May 17, 2016
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.

3 participants