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

Mesh3d colorscale settings #1719

Merged
merged 8 commits into from
Jun 2, 2017
Merged

Mesh3d colorscale settings #1719

merged 8 commits into from
Jun 2, 2017

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented May 23, 2017

@etpinard etpinard force-pushed the mesh3d-colorscale-settings branch from 0982ead to 60b4f44 Compare May 23, 2017 21:33
@@ -16,6 +16,8 @@
[1, "rgb(0, 0, 255)"]
],
"intensity": [0, 0.33, 0.66, 1],
"cmin": -0.5,
"cmax": 0.5,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird. That made the axis labels disappear. I might have to revert back those gl-error3d dependencies bump of commit gl-vis/gl-error3d@d756284

Copy link
Contributor Author

Choose a reason for hiding this comment

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

- previously the colorbar scales were wrong (!)
  as mesh3d used heatmap/colorbar which
  expected `zmin` and `zmax` as colorbar bounds.
  These were undefined, leading to colorbar/draw to fill
  them with (incorrect) default values.
@etpinard
Copy link
Contributor Author

etpinard commented Jun 1, 2017

All green on CI, ready for review @alexcjohnson

.then(function() {
_assert([true, 0, 3], [true, 0, 3]);

return Plotly.purge(gd);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why purge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To destroy the webgl context.

We should perhaps add that Plotly.purge statement in destroyGraphDiv?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah interesting... yeah, would be good to put that in destroyGraphDiv - but it's frustrating that deleting gd from the document isn't sufficient to 🔪 the context - I wonder if we can do something to watch for removing gd and purge automatically? If you think that's not a good idea, we should at least make sure this is well documented somewhere prominent.

coerce('showscale', true);
}
else {
colorscaleDefaults(traceIn, traceOut, layout, coerce, {prefix: '', cLetter: 'c'});
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice when you can remove code to add features and fix bugs 🎉

@alexcjohnson
Copy link
Collaborator

So I guess previously these colorscale ranges were erroneously taken from the z span of the data? Looks great! 💃

@etpinard
Copy link
Contributor Author

etpinard commented Jun 2, 2017

So I guess previously these colorscale ranges were erroneously taken from the z span of the data?

Exactly. mesh3d was using the heatmap colorbar logic previously.

@etpinard etpinard merged commit d7e283a into master Jun 2, 2017
@etpinard etpinard deleted the mesh3d-colorscale-settings branch June 2, 2017 14:42
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.

zmin and zmax properties not implemented for mesh3d types colorbar
2 participants