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

Refactor VTKLUSSimulator to accept rot matrices for rendering #168

Closed
NMontanaBrown opened this issue Apr 27, 2021 · 6 comments
Closed

Refactor VTKLUSSimulator to accept rot matrices for rendering #168

NMontanaBrown opened this issue Apr 27, 2021 · 6 comments

Comments

@NMontanaBrown
Copy link
Collaborator

Issue Description

Currently, the set pose method for the VTKLUSSimulator accepts Euler angles, which are then converted into a rot matrix for rendering.

It would be useful to be able to directly pass rot matrices into the renderer, to avoid having to convert rot m -> euler angle -> back into rot m if the user is using rot ms.

@thompson318
Copy link
Collaborator

I can't find the setpose method. I found set_camera_pose inherherited from vtk_overlay_window but that seems to use matrices. Can you point to the setpose method please.

@mianasbat
Copy link
Contributor

mianasbat commented Apr 27, 2021

It is because it is set_pose method I guess which is in
sksurgeryvtk/widgets/vtk_lus_simulator.py
In testing it is tested
tests/camera/test_vtk_camera_model.py and tests/camera/test_lus_simulator.py

@thompson318
Copy link
Collaborator

Found it now, thanks. set_pose, not setpose
https://github.com/UCL/scikit-surgeryvtk/blob/master/sksurgeryvtk/widgets/vtk_lus_simulator.py#L92

@NMontanaBrown
Copy link
Collaborator Author

NMontanaBrown commented Apr 27, 2021

I can work on this issue in my fork - any strong feelings about how to refactor? Thinking of just adding a new class method, like set_pose_with_matrices(), which accepts p2c, l2c, angle_of_handle and pokes the actors, effectively duplicating LOCs: 177-187 and 138 - 148.

@mianasbat @thompson318

@thompson318
Copy link
Collaborator

Try and avoid having 2 methods with duplicate code. You could create set_pose_with_matrices, but then refactor the current set_pose method so all it does is convert the the pose parameters to matrices then calls set_pose_with_matrices

NMontanaBrown added a commit to NMontanaBrown/scikit-surgeryvtk that referenced this issue Apr 27, 2021
…with_matrices method to allow for flexibility in parametrisation
@NMontanaBrown
Copy link
Collaborator Author

@thompson318 see changes in #169

NMontanaBrown added a commit to NMontanaBrown/scikit-surgeryvtk that referenced this issue Apr 27, 2021
…y using matrices versus angle parametrisation
thompson318 added a commit that referenced this issue Apr 27, 2021
thompson318 added a commit that referenced this issue Apr 27, 2021
Issue #168: refactor set_pose method with new set_pose_with_matrices …
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

No branches or pull requests

3 participants