Improving the quality of surface objects to better follow the data by using LCM or highly composite numbers

[archmoj]

Fixes #2713 by increasing the resolution of surface objects.
With previous set up resolution (128x128) surfaces with any remarkable changes in z-axis were not displayed well.
Also to mention: the issue #2208 and #3262 would be fixed in this PR as well.
@etpinard
@alexcjohnson

[etpinard]

Hmm. I was about to ask if you noticed any performance deterioration on your branch. I guess this here answers my question. 🤔

[etpinard]

... would there be a way to increase the resolution just for traces that need it? In other words, is a way to determine if a given z 2D array requires extra resolution?

[archmoj]

One idea could be to allow resetting this resolution via the api.
I can imagine of auto-detect solutions; but considering animations I suggest we keep this constant.

[archmoj]

@etpinard Rather than using 1024, now by simply using 120 instead of `128' the performance is untouched; while the quality of the graphs are remarkably improved. If we want to go further than that the next magic values that could be applied are 180, 240, 360, 720 and 840.

[etpinard]

Fantastic! Could you explain briefly why 120 is better than 128?

[archmoj]

128 = 2⁷ so it is divisible by (1 + 7) numbers {1, 2, 4, 8, 16, 32, 64, 128}
120 = 2³x3¹x5¹ therefore divisible by (1+3)x(1+1)x(1+1) = 16 numbers {1, 2, 3, 4, 5, 6, 8, 10, 12, 15, 16, 20, 24, 30, 60, 120}.
So it is more likely that the refined grid match actual data points in this case.
https://en.wikipedia.org/wiki/Highly_composite_number
Base on this investigation I am now thinking to rewriting the function that refines scale.

[etpinard]

Wow @archmoj is the man! Thanks!

[etpinard]

Impressive work @archmoj this PR is looking very strong 🍾

The new logic looks good to me. Now, would you mind doing some performance testing on ~5 surface mocks, just to make sure we're not slowing things down too much? Using something like

console.time('surface')
Plotly.newPlot(gd, [/* */])
console.timeEnd('surface')

in the browser console would suffice. Thanks in advance!

[archmoj]

Thank you @alexcjohnson for the review. This is pretty much fixed for parametric cases. I was able to create two mocks related to parametric surface and contour precision. Regarding our discussion concerning computing the ranges twice and possibility of using bounds instead of [mid|max]Values, after searching for computeTraceBounds function I noticed that the bounds are computed after the glplot.update in scene.js. However for the case of surface traces we need these values before drawing the object. So I thought we may keep those the way are. I could still try using Lib.aggnums there.
What do you think?

[alexcjohnson]

Regarding our discussion concerning computing the ranges twice ... I could still try using Lib.aggnums there.

There are two separate issues here: DRY 🌴 and performance 🐎.

🌴 we could investigate replacing both of these implementations with aggNums but if it's at all complicated don't worry about it.

🐎 I would just imagine keeping both copies, but short-circuiting the one in scene if it has already been done (and stashed on the trace) in convert.

[archmoj]

@alexcjohnson after investigation of using aggNums and while user raw text data in not passed in rawData, I thought we could keep convert code as it is. On the other hand I did removed the changes previously made to gl3d/scene.js. So this may be ready to merge?

[alexcjohnson]

Looks good to me! 💃 after merging gl-surface3d and updating its ref, and making an issue for #3281 (comment)

[etpinard]

🎉 this will be part of 1.43.0!