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

Projects
None yet
3 participants
@danlipsa
Contributor

danlipsa commented May 19, 2016

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

This comment has been minimized.

Contributor

danlipsa commented May 19, 2016

@danlipsa danlipsa force-pushed the dotted-line branch from 721383f to d5d898b May 19, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 19, 2016

@doutriaux1 @aashish24 please review.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 19, 2016

I'm excited about this one!

@danlipsa danlipsa force-pushed the dotted-line branch from d5d898b to 8fe2da2 May 20, 2016

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 20, 2016

what was the logic behind this before:

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

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 20, 2016

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

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 21, 2016

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

@aashish24

This comment has been minimized.

Contributor

aashish24 commented May 21, 2016

@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")


BUG #1959: Fix memory override for vtkContourFiler in isofillpipeline.
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 danlipsa force-pushed the dotted-line branch from 8fe2da2 to f91b6b8 May 23, 2016

@danlipsa

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

aashish24 commented May 23, 2016

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

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented May 23, 2016

@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

This comment has been minimized.

Contributor

danlipsa commented May 23, 2016

@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

This comment has been minimized.

Contributor

danlipsa commented May 23, 2016

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

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 23, 2016

@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

This comment has been minimized.

Contributor

aashish24 commented May 23, 2016

@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

This comment has been minimized.

Contributor

aashish24 commented May 23, 2016

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

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 23, 2016

@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

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@danlipsa

This comment has been minimized.

Contributor

danlipsa commented May 24, 2016

@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