Skip to content
This repository has been archived by the owner on Jan 4, 2024. It is now read-only.

PhotonPoseEstimator: Move an lvalue reference #13

Closed
wants to merge 2 commits into from

Conversation

auscompgeek
Copy link
Member

@auscompgeek auscompgeek commented Jan 26, 2023

@james-ward found a trick: we can move a non-const lvalue reference as a workaround for pybind11 not allowing to directly bind to a constructor that takes an rvalue reference.

This does leave behind a Python PhotonCamera object whose backing C++ instance has been moved however, making any attempts to use it rather confusing.

The only alternative I can think of is to patch the PhotonPoseEstimator source to take a shared_ptr. Admittedly the deprecated RobotPoseEstimator does the same, and the Java version doesn't have ownership semantics for obvious reasons, so that might be more acceptable.

Fixes: #12
Closes: #14

@auscompgeek auscompgeek requested a review from virtuald January 26, 2023 12:30
@auscompgeek
Copy link
Member Author

auscompgeek commented Jan 26, 2023

The only alternative I can think of is to patch the PhotonPoseEstimator source to take a shared_ptr.

Looks like the sources aren't published to the Maven repo with the expected sources classifier name, so that wouldn't be a very sustainable approach in the near future. I've raised a PR to fix up the name on their end to be consistent: PhotonVision/photonvision#765

@auscompgeek
Copy link
Member Author

Looks like the sources aren't published to the Maven repo with the expected sources classifier name, so that wouldn't be a very sustainable approach in the near future.

Oh, I see robotpy-build actually has a thing to accommodate for this inconsistency, ha. I'll try putting together a patch.

@auscompgeek
Copy link
Member Author

auscompgeek commented Jan 26, 2023

I'll try putting together a patch.

Nope. 🙃

build/temp.linux-x86_64-cpython-310/dlsrc/photonvision/photonlib/PhotonCamera.cpp:30:10: fatal error: PhotonVersion.h: No such file or directory
   30 | #include "PhotonVersion.h"
      |          ^~~~~~~~~~~~~~~~~

(That's before patches.)

Filed an issue: PhotonVision/photonvision#770

@auscompgeek
Copy link
Member Author

auscompgeek commented Jan 28, 2023

@virtuald any thoughts on moving and poisoning objects?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PhotonPoseEstimator is unavailable
1 participant