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: Fix memory override for vtkContourFiler in isofillpipeline. #1983

Merged
merged 2 commits into from
May 24, 2016

Conversation

danlipsa
Copy link
Contributor

This resulted in vtkStripper to generate double coverage of isocountours which resulted in
messed-up patterns.
Also adjusted plot patterns to easier to discriminate.

@danlipsa
Copy link
Contributor Author

Data in:
CDAT/uvcdat-testdata#133

@danlipsa
Copy link
Contributor Author

@doutriaux1 @aashish24 please review.

@doutriaux1
Copy link
Contributor

I'm excited about this one!

@aashish24
Copy link
Contributor

aashish24 commented May 20, 2016

what was the logic behind this before:

cot.SetValue(numLevels, l[-1])

@danlipsa
Copy link
Contributor Author

This writes over the end of the array.
Not sure what was the original intention.

@aashish24
Copy link
Contributor

aashish24 commented May 21, 2016

Yeah it seems weird, I couldn't think of why it was like this.

@aashish24
Copy link
Contributor

@danlipsa you want to use regression module.

so instead of this

+
 +x = vcs.init(bg=1, geometry=(1620, 1080))
 +x.setantialiasing(0)
 +x.drawlogooff()

You can do:

x = regression.init(bg=1, geometry=(800, 600)) # Let's use 800x600 from now on
....
..
..
regression.run(x, "filename")


This resulted in vtkStripper to generate double coverage of isocountours which resulted in
messed-up patterns.
Also adjusted plot patterns to easier to discriminate.
@danlipsa
Copy link
Contributor Author

danlipsa commented May 23, 2016

@aashish24 Should we do 800 x 600 even for background images? That seems very small considering that we can make images as big as we want. For now I left the image as it was.

@aashish24
Copy link
Contributor

@danlipsa if we make images smaller for testing, then we will have faster checkout of baselines. In vtk the sizes are 400, 400.

@doutriaux1
Copy link
Contributor

@danlipsa @aashish24 I don[t think making the picture smaller will make the test much faster, but we will lose precision in the output and we might not be able to catch small difference, in things like dotted line and such. Just my two cents.

@danlipsa
Copy link
Contributor Author

@aashish24 @doutriaux1 Indeed, for this particular test, 800 x 600 does not catch the difference between a wrong file and a good one - the difference is about 5.

@danlipsa
Copy link
Contributor Author

For the current resolution the difference is 20 is the error is caught.

@danlipsa
Copy link
Contributor Author

@aashish24 Should we merge this in? Note the many sizes we currently have in the repo:
250 x 500
400 x 768
400 x 800
500 x 250
500 x 500
800 x 400
800 x 600
814 x 303
814 x 606
814 x 628
1091 x 1200
1200 x 1090
1200 x 1091
2400 x 2182

@aashish24
Copy link
Contributor

@danlipsa in that case can we use 1200 x 1091 (the default size in regression.py) unless you don't think it catches the error. I see your point that we have different size for testing right now but most of them are using regression.py so it would be nice if we can use 1200x1091.

@aashish24
Copy link
Contributor

but I am fine if the current resolution is best for the testing too 👍

@danlipsa
Copy link
Contributor Author

@doutriaux1 @aashish24 1200 x 1091 is fine. In that case we should look at the baselines of size 1200 x 1090 and make sure we don't compare them against images of size 1200 x 1091.

@danlipsa danlipsa merged commit abebe40 into master May 24, 2016
@danlipsa
Copy link
Contributor Author

@doutriaux1 @aashish24 Note line stipples are not supported yet in OpenGL2 backend for VTK because they have been removed from OpenGL 3.1.
http://www.vtk.org/Bug/view.php?id=15799

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.

3 participants