-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Volume - new gl3d trace #3488
Volume - new gl3d trace #3488
Conversation
package.json
Outdated
@@ -77,7 +77,7 @@ | |||
"gl-heatmap2d": "^1.0.5", | |||
"gl-line3d": "^1.1.8", | |||
"gl-mat4": "^1.2.0", | |||
"gl-mesh3d": "^2.0.7", | |||
"gl-mesh3d": "git://github.com/gl-vis/gl-mesh3d.git#a361fdeae2fcaeed52efe05906f212e4b7c7bafa", |
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.
Interesting view on isosurface vs volume plots by a lab at Lawrence Livermore: |
src/traces/volume4d/convert.js
Outdated
var height = this.data._Ys.length; | ||
var depth = this.data._Zs.length; | ||
|
||
var i = findNearestOnAxis(x, this.data._Xs).id; |
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.
... and "nearest" here means nearest on the "top" slice, correct?
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.
Correct.
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.
... which I think is ok for the first iteration, but could be potentially improved in the future.
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.
When possible, would you mind hovering on gl3d_volume4d_airflow
in your browser?
@archmoj do you have any more screenshots of the state of the art of this trace type so far? |
test/image/compare_pixels_test.js
Outdated
mockName === 'font-wishlist' || | ||
mockName.indexOf('gl3d_volume_') !== -1 || |
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.
Hmm. That's less than ideal. Do you have an idea why we can't pixel-compare volume
mocks?
Moreover, have you tried generating the volume
baseline using orca
?
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 think there is one mock that caused a problem. Now with @antoinerg parallel PR in place I could fix this.
Second question: No I have not tried volume
with orca
.
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.
Second question: No I have not tried
volume
withorca
.
Cool, could you give it a try?
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.
See
Lines 31 to 38 in c657348
test_image () { | |
$root/../orca/bin/orca.js graph \ | |
$root/test/image/mocks/mapbox_* \ | |
--plotly $root/build/plotly.js \ | |
--mapbox-access-token "pk.eyJ1IjoiZXRwaW5hcmQiLCJhIjoiY2luMHIzdHE0MGFxNXVubTRxczZ2YmUxaCJ9.hwWZful0U2CQxit4ItNsiQ" \ | |
--output-dir $root/test/image/baselines/ \ | |
--verbose || EXIT_STATE=$? | |
} |
for a sample orca command.
} | ||
}; | ||
|
||
function isValidScaleArray(scl) { |
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.
Very nicely done 🥇
@archmoj could you tell us a little bit more about how your |
…ojection type to orthographic in one mock
That was basically the result of increasing the waiting time between the batches. |
var Registry = require('../../registry'); | ||
var colorscaleDefaults = require('../../components/colorscale/defaults'); | ||
|
||
module.exports = function supplyIsoDefaults(traceIn, traceOut, defaultColor, layout, coerce) { |
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.
Nicely done! But I would prefer having this supplyIsoDefaults
in isosurface/defaults.js
.
We're trying to move away form this pattern where each defaults-related functions would get their own files. We now prefer having just one defaults.js
file per trace module and have its reusable pieces exported along with the main trace supplyDefaults.
function. See box/defaults.js
for an example.
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.
Done in af17adc.
@archmoj can you add |
Yes. Thanks for the link. |
This PR is looking good! @archmoj two things:
Thanks very much! |
Thanks for the link. From that link I also found this interesting link: It looks that the way matlab does this require multiple steps to create the patch. Since our |
@archmoj the Do we have to test such large mocks in order to 🔒 down the behaviour? |
Should we rename the mocks? |
No, I think we should reduce the size of their data arrays. |
Are you having issues running orca? |
Yes - I needed to reinstall it. But then it didn't work : ( |
OK. I will take care of that. |
@etpinard I was able to test |
Supersedes #3479 initial prototype.
New trace type for
volume
data visualisation.Here is an interactive demo
Note: now
opacityscale
attribute options could be applied to emphasize on maximum or minimum values as well as bothextremes
.