Skip to content

ENH: Plot with vertex normals #78

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

Merged
merged 5 commits into from
Jan 16, 2014
Merged

ENH: Plot with vertex normals #78

merged 5 commits into from
Jan 16, 2014

Conversation

larsoner
Copy link
Contributor

Makes the surfaces much smoother. Although the effect is more striking when zoomed in, it can even be seen zoomed-out when comparing old and new:
OLD:
old

NEW:
new

I used the optimized code from mne-python to do the normal calculations.

@mluessi
Copy link
Contributor

mluessi commented Jan 15, 2014

LGTM :)

@larsoner
Copy link
Contributor Author

I figured if we went through the pains of optimizing the heck out of the normal calculations and of figuring out how to put them in Mayavi, we might as well :) Even if it's likely to be moot in the next 6-12 months assuming vispy makes progress...

@mwaskom
Copy link
Member

mwaskom commented Jan 15, 2014

Oh, fantastic!

@mwaskom
Copy link
Member

mwaskom commented Jan 15, 2014

Maybe some tests for the computation code?

@larsoner
Copy link
Contributor Author

Sure, I could add those tomorrow.

@agramfort
Copy link
Contributor

really cool !

@larsoner
Copy link
Contributor Author

@mwaskom test added, takes 8 sec on my system unfortunately.

@mwaskom
Copy link
Member

mwaskom commented Jan 15, 2014

Does it have to run on the whole surface? Or could you select a subset of the mesh

@larsoner
Copy link
Contributor Author

I could select a subset of the mesh, just a minute.

@mwaskom
Copy link
Member

mwaskom commented Jan 15, 2014

Also probably worth testing the fast cross product against the numpy.cross, just to be safe.

@larsoner
Copy link
Contributor Author

@mwaskom np.cross is implicitly tested by the function:

https://github.com/nipy/PySurfer/pull/78/files#diff-65435034261db5909578ccc0cfe6a83fR19

@larsoner
Copy link
Contributor Author

Okay, made the test (much) faster by saying there were only 100 triangles. Should be ready for review / merge.

@mwaskom
Copy link
Member

mwaskom commented Jan 16, 2014

@mwaskom np.cross is implicitly tested by the function

In the new version, the faster cross product code doesn't actually get exercised as there are only 100 triangles right? In either case, probably better to have a dedicated test just so it's more obvious what's going on if it starts failing.

@larsoner
Copy link
Contributor Author

Explicit _fast_cross_3d test added.

mwaskom added a commit that referenced this pull request Jan 16, 2014
ENH: Plot with vertex normals
@mwaskom mwaskom merged commit d77cea2 into nipy:master Jan 16, 2014
@larsoner larsoner deleted the vert-norm branch March 11, 2014 14:43
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.

4 participants