Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix wrapping of double & output arguments #684

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

acoussat
Copy link
Collaborator

The problem is that in the method rtkConvexShape::IsIntersectedByRay(const PointType &, const VectorType &, double &, double &), the last two parameters are meant to be output values, and thus cannot be used in Python, since there is no way to define a double & in Python.

The solution, crafted thanks to the pointers given by @SimonRit and this comment from @LucasGandel, removes the last two parameters from the method in Python and replaces them with output values (see example below). There might be a better way of achieving the same result, but I am far from fluent in SWIG, so any suggestion of improvement is welcome!

Test script:

import itk
from itk import RTK as rtk

q = rtk.QuadricShape.New()

# Define a sphere of radius 10
radius = 10
q.SetA(1)
q.SetB(1)
q.SetC(1)
q.SetJ(-radius**2)

print(q.IsIntersectedByRay([-20,0,0],[1,0,0]))  # only two parameters

Output with the fix:

[True, 10.0, 30.0]

Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great work. I'm not fluent in swig so I trust it's very well written :-). However, I speak a bit of cmake and it is meaning less to use configure_file here as there is nothing to configure (cmake variables with @, see other .in files)... Can you rename ConvexShape.i.in to ConvexShape.i and replace ${CMAKE_CURRENT_BINARY_DIR}/ConvexShape.i by ${CMAKE_CURRENT_SOURCE_DIR}/ConvexShape.i please?

@acoussat acoussat force-pushed the wrap-isintersectedbyray branch from d4b85c8 to bb19883 Compare February 20, 2025 17:27
@acoussat
Copy link
Collaborator Author

Thanks for the comment. As we discussed, we can also implement the same fix for all the relevant double & in RTK. You told me about the ones in rtkFieldOfViewImageFilter::ComputeFOVRadius, shall we prepare a small list? We can also deal with int & or float & if they also appear.

@SimonRit
Copy link
Collaborator

Thanks for the comment. As we discussed, we can also implement the same fix for all the relevant double & in RTK. You told me about the ones in rtkFieldOfViewImageFilter::ComputeFOVRadius, shall we prepare a small list? We can also deal with int & or float & if they also appear.

I used the following search grep -rE '\s*(short|double|int|float|string) *&' include/*h -B2 and found in addtion to IsIntersectedByRay (excluding const ones)

  • rtk::FieldOfViewImageFilter::ComputeFOVRadius
  • rtk::ThreeDCircularProjectionGeometry::FixAngles

@SimonRit
Copy link
Collaborator

Please also rebase on master to fix the CI.

@acoussat acoussat force-pushed the wrap-isintersectedbyray branch from bb19883 to 3007c9e Compare February 26, 2025 18:27
@acoussat acoussat changed the title Fix wrapping of rtkConvexShape::IsIntersectedByRay Fix wrapping of double & output arguments Feb 26, 2025
@acoussat
Copy link
Collaborator Author

I haven't completely tested it, maybe I first try to make sure that it works properly before merging it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants