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

Make surface contour levels configurable #3469

Merged
merged 16 commits into from Apr 5, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jan 23, 2019

Attempt to address original ticket #485 to make surface contours adjustable and aligned with heatmap & contour trace.
In this PR contour.start, contour.end and contour.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

@archmoj archmoj changed the title Implementation of contours on desired spatial or relative surface locations Surface contours on desired world locations and/or relative positions between grid points Jan 29, 2019
@etpinard
Copy link
Contributor

etpinard commented Feb 12, 2019

Hmm. In #485, we propose making surface contour settings on-par with the attributes for contour traces. @archmoj are your new attributes equivalent to contour's

autocontours, ncontours, contour.start, contour.end and contour.size

? If not, could you explain why your new attributes make more sense for surface traces.

@archmoj
Copy link
Contributor Author

archmoj commented Feb 12, 2019

Hmm. In #485, we propose making surface contour settings on-par with the attributes for contour traces. @archmoj are your new attributes equivalent to contour's

autocontours, ncontours, contour.start, contour.end and contour.size

? If not, could you explain why your new attributes make more sense for surface traces.

I thought this setup could provide more flexibility so that the users can put the contours where they may need.
For example if they want them to be non-uniformly spaced, or in case of skipping one or few contours from the list, it is as simple as providing an array of desired world locations (i.e. similar to iso-surface slices).
Also in regards to @alexcjohnson's comment #485 (comment) similar options are provided for relative locations on/between grid points.

@etpinard
Copy link
Contributor

I would prefer reaching parity with the attributes for contour, but I could be convinced otherwise.

@archmoj
Copy link
Contributor Author

archmoj commented Feb 12, 2019

I would prefer reaching parity with the attributes for contour, but I could be convinced otherwise.

Would you like me to implement ncontours, contour.start, contour.end and contour.size as well?
That's relatively easy.

@alexcjohnson
Copy link
Contributor

I would prefer reaching parity with the attributes for contour, but I could be convinced otherwise.

Would you like me to implement ncontours, contour.start, contour.end and contour.size as well?
That's relatively easy.

Definitely - the primary goal should be that you can take anything that works in a type: 'contour' trace, change just the type attribute to 'surface', and it still displays all the same contours.

Then at some point we can extend BOTH of these to support an array of contour values - perhaps contour.values - for the nonuniform case.

@etpinard etpinard removed this from the v1.45.0 milestone Feb 12, 2019
'Must be positive.'
].join(' ')
},
locations: {
Copy link
Contributor

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.

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. Let's put this on 1.47.0.

Copy link
Contributor Author

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.

@archmoj archmoj changed the title Surface contours on desired world locations and/or relative positions between grid points Make surface contour levels configurable Apr 4, 2019
useNewLevels[i] = true;

for(j = this.contourStart[i]; j < this.contourEnd[i]; j += this.contourSize[i]) {
value = j * this.scene.dataScale[i];
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@etpinard
Copy link
Contributor

etpinard commented Apr 5, 2019

It would've been nice to bring this a bit more on-par with contour, but I guess that can wait for the regl-rewrite.

This will be a nice feature for 1.47.0 💃

@etpinard etpinard added this to the v1.47.0 milestone Apr 5, 2019
@archmoj archmoj merged commit ad9ebda into master Apr 5, 2019
@archmoj archmoj deleted the fix0548-surface-contour-all-points branch April 5, 2019 16:35
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