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

fixed issue with cst orientation in bundle_extraction example #1681

Merged
merged 4 commits into from Dec 9, 2018

Conversation

@BramshQamar
Copy link
Contributor

BramshQamar commented Dec 5, 2018

No description provided.

@arokem
Copy link
Member

arokem left a comment

Great! I think that the code order got a bit mixed along the way. See suggested fix in the comment.

Could you do something similar with the arcuate bundle, so we view that one from the side as well?

window.record(ren, out_path='CST_L_recognized_bundle.png',
size=(600, 600))

This comment has been minimized.

@arokem

arokem Dec 5, 2018

Member

I believe these lines should be:

ren.set_camera(focal_point=(-18.17281532, -19.55606842, 6.92485857),
               position=(-360.11, -340.46, -40.44),
               view_up=(-0.03, 0.028, 0.89))

window.record(ren, out_path='CST_L_recognized_bundle.png', size=(600, 600))

This comment has been minimized.

@BramshQamar

BramshQamar Dec 8, 2018

Contributor

Hello Ariel,

Thanks for point out the typo. I have fixed it and also camera view for AF_L bundle.

here's how they look now:
af_l_recognized_bundle

cst_l_recognized_bundle

This comment has been minimized.

@BramshQamar
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 6, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@96d0e73). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1681   +/-   ##
=========================================
  Coverage          ?   90.55%           
=========================================
  Files             ?      232           
  Lines             ?    28250           
  Branches          ?     2995           
=========================================
  Hits              ?    25582           
  Misses            ?     2055           
  Partials          ?      613

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96d0e73...253e47d. Read the comment docs.

ren.set_camera(focal_point=(-18.17281532, -19.55606842, 6.92485857),
position=(-360.11, -340.46, -40.44),
view_up=(-0.03, 0.028, 0.89)), size=(600, 600))
size=(600, 600))
if interactive:
window.show(ren)

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2018

Member

I believe in show and record you have to say reset_camera=False

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2018

Member

@BramshQamar let me know if this is not necessary.

This comment has been minimized.

@BramshQamar

BramshQamar Dec 8, 2018

Contributor

It works fine without setting reset_camera=False.
if I set it to False then our streamlines appear to be very far and zoomed out.

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 8, 2018

Member

I believe still fury updates the camera automatically when reset_camera=True. Only for scaling though.

This comment has been minimized.

@BramshQamar

BramshQamar Dec 8, 2018

Contributor

if I do reset_camera=False

this is how it saves the .png

af_l_recognized_bundle

This comment has been minimized.

@BramshQamar

BramshQamar Dec 8, 2018

Contributor

we cannot see anything

This comment has been minimized.

@arokem

arokem Dec 8, 2018

Member

I can't see anything in there? 😄

Seems like maybe a bug in fury?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 8, 2018

Looks great! One more tiny thing: I think that we want to set leave interactive=False per default (on L118 and on L154).

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 9, 2018

LGTM. Merging.

I also posted an issue on Fury, so we can resolve the reset_camera conundrum there.

@arokem arokem merged commit 98e2052 into nipy:master Dec 9, 2018

4 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 9, 2018

Thanks for doing this!

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 9, 2018

You need to call clipping_range too and make sure that the camera is looking at the object correctly.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 9, 2018

@Garyfallidis : what's wrong with the currently produced images? Looks like the FOV is quite good?

cst_l_recognized_bundle
af_l_recognized_bundle

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 9, 2018

There is nothing wrong. I am explaining a bit what is the standard procedure when you do not want to have fury generating automatically camera parameters. It might be useful for other bundles.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 9, 2018

No issue for this being merged.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Dec 9, 2018

Gotcha! Should we add more details about how to do this correctly in the bundle visualization example? Maybe make an issue about this? Should be a relatively low-hanging fruit for relatively new contributors.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Dec 9, 2018

It's fine. Let's move on. That clarification should go to fury and you already reported an issue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment