Closed danlipsa closed 5 years ago
@doutriaux1 @aashish24 @scottwittenburg Please review. Differences between old and new baselines are caused by:
Thanks for the pointers Dan. I think the volume link is this one?
https://106-68308410-gh.circle-artifacts.com/0/tests_html/test_dv3d_volume_test.html
It's hard to tell what's what without a color legend in the images, but I like the new images better. Also, it seems to make sense that just turning on a light could change the appearance of the slices like that. I vote for replacement of the baselines.
Thanks for taking a look Scott. I think the link you sent highlights another problem with the old code. The transparent voxels don't seem to be ordered correctly along the depth axis.
Good observation. Yes, I think picking up the latest VTK has brought some nice improvements overall, and I think we should update the baselines and merge this. But let's see what @doutriaux1 thinks.
+1
@danlipsa https://109-68308410-gh.circle-artifacts.com/0/tests_html/test_dv3d_hovmoller_test.html looked a bit better before I think. But I could live with it.
Same for: https://109-68308410-gh.circle-artifacts.com/0/tests_html/test_dv3d_sliders.html
Why are they darker now?
https://109-68308410-gh.circle-artifacts.com/0/tests_html/test_dv3d_volume_test.html looks very different now.
I suspect, based on Dan's comment about how the old code didn't have lighting, that the darkness comes from the light direction being nearly perpendicular to the brighter plane (and thus not shining much on the darker plane). If we could move the light a little it might make the bright plane a little darker and the dark plane a little brighter. But I'm not sure we have easy control over that in dv3d
.
However, I'm just guessing about all of that.
@scottwittenburg that sounds good. Let's not worry about it
@doutriaux1 Scott is right. In the past, the code had ambient light = 1 and diffuse light = 0. So light didn't change based on the light direction. I could make it that way but I think it is better the way it is now.
@scottwittenburg @danlipsa I agree. Although could we make it a parameter?
I am sure we could make it a parameter, but this would make the code harder to maintain. (I think we already have too many parameters in vcs). The images are only really dark when seen at very sharp angles when only very little of the image is seen. That gives you the 3D depth feeling so its not a bad thing. So I would vote to not add an extra parameter.
I experimented with ambient light = 0.2, that brightens all images but also changes slightly how the colors look like in https://142-68308410-gh.circle-artifacts.com/0/tests_html/test_dv3d_hovmoller_test.html and thats not a good thing. So ambient light = 0 seems the right thing to do.
@danlipsa ok let's go with that for now. I will forward your tests to the team and collect answers though.
@danlipsa what about: https://106-68308410-gh.circle-artifacts.com/0/tests_html/test_dv3d_volume_test.html
Why are they so different?
@doutriaux1 My reading on this is that the transparent voxels don't seem to be ordered correctly. You see patterns from deep inside that should be less visible because they are obscured by voxels closer to the viewer. It may also be the sampling rate: sampling less often results in more patterns.
@aashish24 @sankhesh What do you think?
@danlipsa I think the opacity accumulation is out of whack. Tweak vtkVolumeProperty::SetScalarOpacityUnitDistance
@doutriaux1 So there are two parameters that Sankhesh helped me with: One is UseJitteringOn (default now, probably non existent in the past) https://www.vtk.org/doc/nightly/html/classvtkGPUVolumeRayCastMapper.html This is the image I got with UseJitteringOff So this parameter is used to get rid of wood grain artifacts.
A second parameter just makes the image more transparent: SetScalarOpacityUnitDistance(1.5) (the default is 1) https://www.vtk.org/doc/nightly/html/classvtkVolumeProperty.html
So, we could leave the image as is - and let the user adjust the opacity using the opacity transfer function, or we could make it a little more transparent to better match what we had before. I think the jittering should be on because, those extra feature are not existing in the data.
@doutriaux1 Are we good here?
@danlipsa I think we are. But I will merge once @scottwittenburg is done with vcs and we're ready to bring everything in. Did anybody (I'm guilty here) had a chance to look at vcsaddons?
@doutriaux1 I'll take a look at vcsaddons today.
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
DV3D/ImagePlaneAxisAlignedWidget.py | 0 | 1 | 0.0% | ||
DV3D/ImagePlaneWidget.py | 0 | 2 | 0.0% | ||
DV3D/ListWidget.py | 0 | 2 | 0.0% | ||
DV3D/DV3DPlot.py | 0 | 3 | 0.0% | ||
DV3D/RectilinearGridPlot.py | 0 | 9 | 0.0% | ||
DV3D/ButtonBarWidget.py | 0 | 12 | 0.0% | ||
<!-- | Total: | 0 | 29 | 0.0% | --> |
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
DV3D/ImagePlaneAxisAlignedWidget.py | 1 | 0.0% | ||
DV3D/ImagePlaneWidget.py | 1 | 0.0% | ||
<!-- | Total: | 2 | --> |
Totals | |
---|---|
Change from base Build 155: | 0.0% |
Covered Lines: | 0 |
Relevant Lines: | 13764 |
Updated baselines in: https://github.com/CDAT/uvcdat-testdata/pull/200