Skip to content

BUG: Fix memory override for vtkContourFiler in isofillpipeline.#1983

Merged
danlipsa merged 2 commits intomasterfrom
dotted-line
May 24, 2016
Merged

BUG: Fix memory override for vtkContourFiler in isofillpipeline.#1983
danlipsa merged 2 commits intomasterfrom
dotted-line

Conversation

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

Data in:
CDAT/uvcdat-testdata#133

@danlipsa
Copy link
Copy Markdown
Contributor Author

@doutriaux1 @aashish24 please review.

@doutriaux1
Copy link
Copy Markdown
Contributor

I'm excited about this one!

@aashish24
Copy link
Copy Markdown
Contributor

aashish24 commented May 20, 2016

what was the logic behind this before:

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

@danlipsa
Copy link
Copy Markdown
Contributor Author

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

@aashish24
Copy link
Copy Markdown
Contributor

aashish24 commented May 21, 2016

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

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

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

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

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

@danlipsa
Copy link
Copy Markdown
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
Copy Markdown
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