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

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

Merged
merged 1 commit into from Nov 23, 2015

Conversation

danlipsa
Copy link
Contributor

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
Copy link
Contributor Author

@sankhesh @aashish24 @doutriaux1 Please review.

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but @sankhesh should approve it.

@sankhesh
Copy link
Contributor

@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.

@doutriaux1
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

@chaosphere2112
Copy link
Contributor

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

@doutriaux1
Copy link
Contributor

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

@danlipsa
Copy link
Contributor Author

@doutriaux1 @chaosphere2112 is this good to go?

@danlipsa
Copy link
Contributor Author

@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
Copy link
Contributor

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

@aashish24
Copy link
Contributor

@danlipsa were you able to fix the legend?

@danlipsa
Copy link
Contributor Author

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

@aashish24
Copy link
Contributor

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

@danlipsa
Copy link
Contributor Author

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

@danlipsa
Copy link
Contributor Author

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

@sankhesh
Copy link
Contributor

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

@danlipsa
Copy link
Contributor Author

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

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.
@chaosphere2112
Copy link
Contributor

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
BUG #1645: Blank spots on solid fillareastyle with fillareaindices
@chaosphere2112 chaosphere2112 merged commit 5ea46aa into master Nov 23, 2015
@chaosphere2112
Copy link
Contributor

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

@chaosphere2112
Copy link
Contributor

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

@chaosphere2112 chaosphere2112 deleted the blank-spots branch November 23, 2015 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants