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

Fix Pybind11 Imath Frustum #459

Merged
merged 19 commits into from
Dec 19, 2024

Conversation

TodicaIonut
Copy link
Contributor

  • The code had "Imath::" where it should have had "IMATH_NAMESPACE::

S is not declared. Same thing in subsequent lines.

not working

Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

Thanks for the speedy response, and yes, the previous PR replaced the Imath:: with ``IMATH_NAMESPACE::` to support the case where Imath is built with a custom namespace.

Looks like the template is still not quite right here.

void register_frustum(pybind11::module& m, const char *name)
{
pybind11::class_<T> c(m, name);
c.def(pybind11::init<>(), "Uninitialized by default")
.def(pybind11::init<T>(), pybind11::arg("frustum"), "Copy constructor")
.def(pybind11::init<S>(), pybind11::arg("nearPlane"), pybind11::arg("farPlane"), pybind11::arg("fovx"), pybind11::arg("aspect"), "Initialize with basic frustum properties")
.def(pybind11::init<const S, S, S, S>(), pybind11::arg("nearPlane"), pybind11::arg("farPlane"), pybind11::arg("fovx"), pybind11::arg("aspect"), "Initialize with basic frustum properties")
Copy link
Member

Choose a reason for hiding this comment

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

nearPlane, farPlane, fovx, and aspect are float/double, but S in this context is M44f/M44d, right?

Signed-off-by: Todica Ionut <[email protected]>
@cary-ilm
Copy link
Member

Looks like you added a 4th argument to the template instantiation but not the declaration, the build is still failing. Is this somehow compiling on your end?

Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
Signed-off-by: Todica Ionut <[email protected]>
@TodicaIonut
Copy link
Contributor Author

Ci temporarily:

  1. Remove Frustum build failing
  2. Remove Euler build Test failing
ImportError: arg(): could not convert default argument 'order: Imath_3_2::Euler<float>::Order' in method '<class 'pybindimath.Eulerf'>.__init__' into a Python object (type not registered yet?)
Error: Process completed with exit code 1.

@cary-ilm I merged. CI checks failed

Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

This gets the build working again, I'm going to merge as is. I'll be away from the project Dec 19-Jan 12, but can help resolve the issues after that.

@cary-ilm cary-ilm merged commit aa28eb5 into AcademySoftwareFoundation:main Dec 19, 2024
25 checks passed
@TodicaIonut TodicaIonut deleted the patch-1 branch December 19, 2024 06:48
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.

2 participants