search

AprilRobotics / apriltag

AprilTag is a visual fiducial system popular for robotics research.
https://april.eecs.umich.edu/software/apriltag
Other
1.47k stars 522 forks source link

declare binary parameters as bool #250

Closed christian-rauch closed 1 year ago

christian-rauch commented 1 year ago

Some of the apriltag_detector_t parameters are meant to be used as binary flags but stored as an integer with a much larger value range. This makes it difficult from an API perspective to directly grasp the meaning of a parameter. I am proposing here to change the type of refine_edges, debug and qtp.deglitch from int to bool.

While this is an API change, the impact should be low since previously used ints should be cast automatically for boolen values.

christian-rauch commented 1 year ago

@wxmerkt I bumped the version to 3.3 as we accumulated quite a few changes since the last release and the API changed slightly. After this has been merged, I (or you) would push a new tag to the repo.

Regarding the bloom releases, are those released automatically to rolling once the repo has been tagged or does someone from the release team have to trigger a new bloom release?

wxmerkt commented 1 year ago

Regarding the bloom releases, are those released automatically to rolling once the repo has been tagged or does someone from the release team have to trigger a new bloom release?

We need to run releases manually - ideally to all distros. Beyond an API change, I suspect this may also introduce ABI changes which may cause issues with downstream packages unless rebuilt?

christian-rauch commented 1 year ago

Regarding the bloom releases, are those released automatically to rolling once the repo has been tagged or does someone from the release team have to trigger a new bloom release?

We need to run releases manually - ideally to all distros. Beyond an API change, I suspect this may also introduce ABI changes which may cause issues with downstream packages unless rebuilt?

Yes, I also think there may be ABI changes. I am not sure how compilers represent bool values; it could be int, in which case this would still work. If in doubt, I would just re-release the downstream packages.

wxmerkt commented 1 year ago

We cannot just re-release downstream packages since it's such a widely used package.

Also, I believe some of the motivation behind avoiding the use of bool might have been a desire to have compatibility with old C standards (perhaps for some embedded applications?). We might have to note the minimum requirement as such in the README/CMake settings if not set already.