-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Make surface contour levels configurable #3469
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
Conversation
…isable nticks if false draw ntick contours
Hmm. In #485, we propose making surface contour settings on-par with the attributes for
? If not, could you explain why your new attributes make more sense for |
I thought this setup could provide more flexibility so that the users can put the contours where they may need. |
I would prefer reaching parity with the attributes for |
Would you like me to implement |
Definitely - the primary goal should be that you can take anything that works in a Then at some point we can extend BOTH of these to support an array of contour values - perhaps |
src/traces/surface/attributes.js
Outdated
'Must be positive.' | ||
].join(' ') | ||
}, | ||
locations: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need locations
and relative
to ✅ this feature?
I would prefer making surface
on-par with contour
to start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Let's put this on 1.47.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped those features in 98aebc5 as surface
code is already pretty complicated for us to maintain.
useNewLevels[i] = true; | ||
|
||
for(j = this.contourStart[i]; j < this.contourEnd[i]; j += this.contourSize[i]) { | ||
value = j * this.scene.dataScale[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried re-using setContours
? It probably gives similar results, but it would be nice to double-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that on a different branch. Also wanted to add autocontours
and ncontours
in this PR.
But unfortunately that could add too much complexity while surface contours are in 3 directions not only z.
It would've been nice to bring this a bit more on-par with This will be a nice feature for 1.47.0 💃 |
Attempt to address original ticket #485 to make surface contours adjustable and aligned with
heatmap
&contour
trace.In this PR
contour.start
,contour.end
andcontour.size
are added and could be applied by the user to slice surface graphs on desired positions other than tick positions which is the default behaviour.Codepen example
@plotly/plotly_js