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

BUG #1645: Blank spots on solid fillareastyle with fillareaindices #1700

Merged
merged 1 commit into from Nov 23, 2015

Conversation

Projects
None yet
5 participants
@danlipsa
Contributor

danlipsa commented Nov 19, 2015

The solid fillareastyle was treated as a pattern style in that
each level was extracted and rendered individually. Because of a
bug in vtkBandedPolyDataContourFilter lower isocontours differ
from upper isocontours which results in blank spots. In this fix
we trigger merging of levels which fixes the problem for solid fill.
We'll fix the problem for pattern fill in a subsequent commit.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 19, 2015

# levels would not be merged if the user sets fillareaindices for a
# 'solid' style resulting in a less efficient plotting.
indices = (None if (style == "solid") else self._gm.fillareaindices)
opacities = self._gm.fillareaopacity

This comment has been minimized.

@aashish24

aashish24 Nov 19, 2015

Contributor

LGTM but @sankhesh should approve it.

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Nov 19, 2015

@danlipsa I would say this is a workaround and the real issue is in vtkBandedPolyDataContourFilter. Fixing the filter would fix this issue for all fill styles.

Question: If the above hypothesis is correct, do we need this workaround?
Running through vtkBandedPolyDataContourFilter once and adding just one renderer to the scene for solid fills would be better in terms of performance and picking.

Moreover, this issue will present even if there is just one index and multiple colors or opacities.

FYI, this code needs to be rebased on master. This calculation was moved to the superclass.

@danlipsa danlipsa force-pushed the blank-spots branch from f34f98c to e4b9bb8 Nov 19, 2015

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Nov 19, 2015

I think @sankhesh is right, beside @chaosphere2112 pointed out that it doesn't work either for hatches and patterns (although it is harder to tell)

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 19, 2015

I think @sankhesh is right, beside @chaosphere2112 pointed out that it doesn't work either for hatches and patterns (although it is harder to tell)

@danlipsa pointed out that this is only for solid fills. I believe @danlipsa is planning to push a separate branch for patterns.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 19, 2015

Indeed, fixing vtkBandedPolyDataContourFilter would fix things for solid fill as well. I think this patch is still worth doing as without it, we do more work than required. There is no need to split the dataset in bands and render each of them individually (for solid fill). We can just render one band and set colors and opacities in the LUT.

@danlipsa danlipsa force-pushed the blank-spots branch from e4b9bb8 to 84ade1d Nov 19, 2015

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 19, 2015

Yeah, I agree; it would be better to not actually bother running through the pattern code with solid fills.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Nov 20, 2015

Broken tests. @chaosphere2112 I will let you take the lead on this and merge if you are OK when tests pass. Thx

@danlipsa danlipsa force-pushed the blank-spots branch from 84ade1d to aeaa530 Nov 20, 2015

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 23, 2015

@doutriaux1 @chaosphere2112 is this good to go?

@danlipsa danlipsa force-pushed the blank-spots branch from aeaa530 to 5063f5c Nov 23, 2015

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 23, 2015

@sankhesh pointed out that I should make the same changes to meshfillpipeline. This new pull request adds this and also adds new tests baselines for meshfill:
CDAT/uvcdat-testdata#87

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 23, 2015

@danlipsa I'll test this today and hopefully merge.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 23, 2015

@danlipsa were you able to fix the legend?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 23, 2015

@aashish24 yes, the problem was with opacities rather than the legend/colors.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Nov 23, 2015

Ah thanks @danlipsa so this will fix the blanking for solids. I am assuming that we still need fix for patterns?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 23, 2015

@aashish24 yes, we still need the fix for patterns however, that bug is not easy to spot. See the following image:
spots_patterns

@danlipsa danlipsa force-pushed the blank-spots branch from 5063f5c to 5afb7cd Nov 23, 2015

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 23, 2015

@sankhesh @aashish24 @chaosphere2112 Please review. It seems that the relevant tests pass.

@sankhesh

This comment has been minimized.

Contributor

sankhesh commented Nov 23, 2015

@danlipsa You might want to increase the image comparison threshold for meshfill tests. See https://open.cdash.org/viewTest.php?onlyfailed&buildid=4115057

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Nov 23, 2015

See the VTK bug report for vtkBandedDataContourFilter. Will said he is going to look at it this week or next.
http://www.vtk.org/Bug/view.php?id=15845

BUG #1645: Blank spots on solid fillareastyle with fillareaindices.
The solid fillareastyle was treated as a pattern style in that
each level was extracted and rendered individually. Because of a
bug in vtkBandedPolyDataContourFilter this results in blank spots. In this fix
we trigger merging of levels which fixes the problem for solid fill.
We'll fix the problem for pattern fill in a subsequent commit.

@danlipsa danlipsa force-pushed the blank-spots branch from 5afb7cd to 833d0a5 Nov 23, 2015

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 23, 2015

I got 100% passed tests, and looking at my test case from #1645, it seems to be fixed. Changes seem straightforward. I'll merge!

chaosphere2112 added a commit that referenced this pull request Nov 23, 2015

Merge pull request #1700 from UV-CDAT/blank-spots
BUG #1645: Blank spots on solid fillareastyle with fillareaindices

@chaosphere2112 chaosphere2112 merged commit 5ea46aa into master Nov 23, 2015

0 of 9 checks passed

cont-int/LLNL/Darwin-Mac (NOGUI) running 'make -j4' (Mon Nov 23 13:23:59 2015)
Details
cont-int/LLNL/Darwin-Mac LEAN running 'make -j4' (Mon Nov 23 13:24:05 2015)
Details
cont-int/LLNL/Darwin-Mac2 10.10.5 (FULL) running 'make -j4' (Mon Nov 23 13:24:47 2015)
Details
cont-int/LLNL/Linux-RH6 (FULL) running 'In Queue: 1' (Mon Nov 23 13:23:31 2015)
Details
cont-int/LLNL/Linux-RH6 (MESA) running 'make -j12' (Mon Nov 23 13:24:03 2015)
Details
cont-int/LLNL/Linux-Ub. 15.10 (FULL/MESA) running 'make -j15' (Mon Nov 23 13:24:04 2015)
Details
continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build started
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 23, 2015

Oh, whoops; probably should have waited till the thresholds got bumped up for the meshfill tests.

@chaosphere2112

This comment has been minimized.

Contributor

chaosphere2112 commented Nov 23, 2015

... or maybe that did get in. Don't mind me, I'm quite insane, you know.

@chaosphere2112 chaosphere2112 deleted the blank-spots branch Nov 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment