Skip to content

Fix numpy_num_face_vertices()#408

Merged
syoyo merged 26 commits intoreleasefrom
numpy-fixes
Mar 3, 2026
Merged

Fix numpy_num_face_vertices()#408
syoyo merged 26 commits intoreleasefrom
numpy-fixes

Conversation

@paulmelnikow
Copy link
Contributor

@paulmelnikow paulmelnikow commented Feb 20, 2026

  • Match type of created py::array_t to the type in the struct.

A follow-up to #405 and #407 which need to land first

Closes #400

@paulmelnikow paulmelnikow changed the base branch from release to upgrade-cibw February 20, 2026 20:17
@paulmelnikow paulmelnikow marked this pull request as ready for review February 20, 2026 20:17
@paulmelnikow
Copy link
Contributor Author

This PR and possibly this branch will need updating once #405 and #407 are merged, but in the meantime it's possible to review this PR.

@paulmelnikow paulmelnikow changed the base branch from upgrade-cibw to release February 23, 2026 21:18
@paulmelnikow
Copy link
Contributor Author

@syoyo Ready for review

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a type mismatch bug in the numpy_num_face_vertices() Python binding that caused incorrect values to be returned. The underlying C++ mesh_t::num_face_vertices field is a std::vector<unsigned int>, but the numpy binding was creating a py::array_t<unsigned char> and using sizeof(unsigned char) for the memcpy operation, resulting in truncated data and zeros appearing in the output.

Changes:

  • Changed numpy_num_face_vertices() to use unsigned int instead of unsigned char to match the underlying C++ type
  • Updated both the py::array_t template parameter and the sizeof() call in memcpy to use the correct type

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI commented Feb 24, 2026

@paulmelnikow I've opened a new pull request, #412, to work on those changes. Once the pull request is ready, I'll request review from you.

@syoyo
Copy link
Collaborator

syoyo commented Mar 3, 2026

@copilot Add regression test(in python layer) for this PR to python. create accompanying .obj file under 'models/' Setup minimal python unit test/regression test runner upon necessary.

Copy link
Contributor

Copilot AI commented Mar 3, 2026

@syoyo I've opened a new pull request, #418, to work on those changes. Once the pull request is ready, I'll request review from you.

@syoyo syoyo merged commit fcc9005 into release Mar 3, 2026
39 checks passed
@syoyo syoyo deleted the numpy-fixes branch March 3, 2026 17:52
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