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

Make sure antialiasing is set right during context creation #1422

Merged
merged 2 commits into from Jun 26, 2015

Conversation

@sankhesh
Copy link
Contributor

@sankhesh sankhesh commented Jun 23, 2015

Fixes issue #1400

@sankhesh
Copy link
Contributor Author

@sankhesh sankhesh commented Jun 23, 2015

@dlonie Please review

@@ -293,7 +295,7 @@ def createRenWin(self,*args,**kargs):
self.renWin.SetStencilCapable(1)
## turning off antialiasing by default
## mostly so that pngs are same accross platforms
Copy link
Contributor

@allisonvacanti allisonvacanti Jun 24, 2015

Let's remove this comment, since it's no longer true after this patch.

@allisonvacanti
Copy link
Contributor

@allisonvacanti allisonvacanti commented Jun 24, 2015

A couple minor issues, but looks good to me otherwise 👍

@sankhesh
Copy link
Contributor Author

@sankhesh sankhesh commented Jun 24, 2015

Implemented @dlonie's suggestions in e1b093d

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jun 24, 2015

Nice!! this will make things look much better. We should send an email once this branch is merged with before and after (pick a crappy looking plot from before)

@allisonvacanti
Copy link
Contributor

@allisonvacanti allisonvacanti commented Jun 24, 2015

Works here! @doutriaux1 Want to test/merge as well? @sankhesh and I are both at Kitware.

@allisonvacanti
Copy link
Contributor

@allisonvacanti allisonvacanti commented Jun 24, 2015

@aashish24 Here are the images I sent you and Brian while investigating this. At full resolultion it's quite an improvement.

No multisampling (current master)
orig

16x MSAA (possible after this patch)
with_msaa

16x MSAA + gm.linewidth = 2 (best match to Brian's sample plots)
with_msaa_linewidth2

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jun 24, 2015

Nice! is 16x MSAA is set by you on your driver or someplace else?

@allisonvacanti
Copy link
Contributor

@allisonvacanti allisonvacanti commented Jun 24, 2015

The argument to Canvas.setantialiasing (and eventually vtkRenderWindow::SetMultiSamples) is the number of samples to take.

So after this patch, users can do:

x = vcs.init()
x.setantialiasing(16) # or 8, or 4, or 2, or 0...
x.plot(...)

and the result will be antialiased. If the graphics hardware/driver doesn't support the number of requested samples, VTK will set it as high as possible for the hardware/driver.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jun 24, 2015

Okay, sounds good to me. Thanks. @doutriaux1, some folks want this today as they producing 1D plots. Would it be possible for you to give it a thumbs up?

@jypeter
Copy link
Member

@jypeter jypeter commented Jun 24, 2015

@aashish24, can you try running the script in #1079 and see if you get nice(r) dots and markers, when using the smoothing?

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jun 24, 2015

great! building now and will merge.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jun 24, 2015

@jypeter I will request @sankhesh to do it. @sankhesh can you post your results here?

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jun 25, 2015

@sankhesh k it works and it's great. Unfortunately for you it's way too great to be left to the user. So can I please ask you to tweak this so that antialiasing is on by default. Of course that means turning it off for all vcs tests... sorry.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jun 26, 2015

@doutriaux1 we cannot turn it ON for testing since it will make resulting image vary depending on the user's setting. I would suggest we merge this one in and have @sankhesh push another branch where we turn it OFF for testing and having it ON by default.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jun 26, 2015

Ooops..sorry I just read your email wrong. I totally agree with you and that's what we were planning to do it (as we do the same for VTK).

aashish24 added a commit that referenced this issue Jun 26, 2015
Make sure antialiasing is set right during context creation
@aashish24 aashish24 merged commit 0ec0506 into master Jun 26, 2015
3 checks passed
@aashish24 aashish24 deleted the antialiasing_issue branch Jun 26, 2015
@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jun 26, 2015

@aashish24 should we open another issue for this then? I was hoping @sankhesh would stick it in this branch.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jun 26, 2015

done, see: #1432

@sankhesh
Copy link
Contributor Author

@sankhesh sankhesh commented Jun 29, 2015

@jypeter Here is a montage of screenshots generated using the script in #1079 using different antialiasing modes:
screenshot from 2015-06-29 11 41 58

No antialiasing:
test_simple_marker_no_msaa

8x MSAA:
test_simple_marker_8x_msaa

16x MSAA:
test_simple_marker

@jypeter
Copy link
Member

@jypeter jypeter commented Jul 1, 2015

Thanks @sankhesh , it does look nicer with 8x and 16x (8x may be enough)!

I don't remember if I have already asked this, but is it possible to generate nicer circles and even triangles from the start (before the smoothing stage)? When you look at the triangle in the "no MSAA" picture above, it looks pixelized, but with much bigger pixels than the pixels in the "trian" string

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jul 1, 2015

@sankhesh it's probably just a matter of tweaking the vtksource object we're using.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jul 1, 2015

@doutriaux1 yes, that should be possible. I will create a new issue for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants