Can we deprecate the Makefile?

[jrepp]

There is further drift been make and cmake versions of the build. CMake is the default working version. If this is agreeable I can open a PR against this issue to fold the Makefile options into the CMake ensuring that we get apples-to-apples build comparison between make and a default cmake build.

[mkrogius]

I want to keep the makefile around in order to support building apriltag even when cmake is not installed, but there is no need to keep the makefile in sync with CMake. Anybody using the makefile should be aware that the results may be slightly different and that they should install cmake to get the best results.

[jrepp]

I'm seeing this as a long-term tax for little benefit. The Makefile creates some non-supported surface area on the project. I'm hoping to add more testing and coverage support through CMake but don't plan to duplicate that work.

I'm ok with closing this issue - I just wanted to raise it as an issue for discussion.

[mkrogius]

I think that makes a lot of sense and that there is no need to duplicate that work for the Makefile

[mkrogius]

I posted the above ^ before reading the other PRs where there are larger changes that would in fact break the makefile and necessitate testing and keeping the makefile up-to-date. Here's my updated thoughts:

Keeping the makefile around has not consumed much dev time so far, because most other changes have been quite limited in scope. I agree that the makefile will be an extra tax if larger changes keep happening. So the question of what to do about the makefile is in some sense a question about the future of this project i.e. should the project stay in maintenance mode. I personally have very little time to devote to code reviews and testing, so my preference would be for the project to remain in maintenance mode with only minor bugfixes/quality-of-life changes accepted to the core of this project. Also, I don't (so far) see any good reason to think that contributed PRs will meaningfully improve the performance (detection distance/speed) of this project so I think that the cost-benefit of accepting large changes is small.

We haven't previously made any statement about the nature of changes that we plan to accept, so I would like to hear other people's opinions on this matter. @acshi @christian-rauch @wxmerkt

[acshi]

As far as this project is currently owned by the April Robotics Lab, I think we should acknowledge that the lab and alums from the lab have very little time to maintain this. So I think the default here should be to stay in a low-cost maintenance mode.

Regardless of that, I think that any changes should be heavily justified, either because of explicit examples where undefined behavior is invoked, or significant performance differences with the benchmarking numbers to prove them.

Glancing at recent issues, for example the diagnostics PR that seems to motivate this, I am not sure the use case for the changes is strongly made. Is there any performance difference? Is some specific use case not possible? The example given is specifying an invalid tag family and getting an assertion abort, which even if not the prettiest behavior, doesn't seem to block any specific use of AprilTag. So I would tentatively say that large-scale refactoring like that is perhaps better suited to a fork of the project... I haven't been very active looking at issues here, but I agree that I think performance and correctness are the main areas of concern, and so I think changes should be justified in those terms.

[jrepp]

I am actively working on an AprilTag based solution for an industrial application for the remainder of this year and probably beyond. I have approval to upstream changes which is my preference so that the broader community can benefit from this work and we don't curate a dead end of an important piece of software.

I'd like to keep contributing and am willing to offer my time as a maintainer if the team is amiable to it. I have experience in maintaining long-lived stable APIs (I was an Xbox platform developer) and doing low-level engine code for video games at companies like Blizzard, MS, and EA. Not to argue from authority just to say I have a lot of experience in writing, maintaining, and optimizing tricky performance-critical code.

Re: Makefile/CMake conversation: I removed most of the diagnostic feature PR to help land it in smaller bite-size pieces but I have more CMake options to control a lot of run-time behavior (logging, image/detection tracing, panic behavior, and developer feature flagging) that I have developed and would love to get those into the hands of other developers. I see some pent-up demand in issues for controlling these things and I've seen forks such as the OpenMV fork that did it in a non-maintainable way.

The code has some interesting ideas commented out that would make for feature flags (instead of if(0) sections) but more than that I think we see some contributions from the community to improve things like pose estimation which would be useful as compile-time feature flags before it makes it to GA. The goal here is to unlock the ability of researchers (both academic and industry) to be able to measure experiments in real-world settings while maintaining stability, parity, and compatibility for users at large.

Here's a handful of things that I'd love to personally contribute to the project:

• Automated acceptance tests, coverage, and performance through GitHub actions
• Improvements to frame detection speed, specifically on constrained ARM7/RISCV based devices
• Reduce dynamic memory usage and improve the overall stability of the library for industrial applications
• Maintain and validate ROS1/2 support
• Port the diagnostics callbacks to ROS topics
• Port the diagnostics callbacks to python
• Improve detection capabilities for non-planar surfaces (we have AprilTags on surfaces that experience mild deformations)

However, this is not my project, I'm just helping out and there is no shame in saying - "we like it how it is!". I will be happy and pursue my goals either way and I have massive respect for the AprilRobotics researchers and all of the developers who make up this corner of the internet :)

Sorry if the scope of the original issue blew up but I do appreciate the conversation.

[jrepp]

Regardless of that, I think that any changes should be heavily justified, either because of explicit examples where undefined behavior is invoked, or significant performance differences with the benchmarking numbers to prove them.

I agree - I think before even pursuing performance-related changes we need better performance baselines and testing methods.

Glancing at recent issues, for example, the diagnostics PR that seems to motivate this, I am not sure the use case for the changes is strongly made. Is there any performance difference? Is some specific use case not possible?

The diagnostics feature PR is more of a pre-curser or enabling branch which enables a few things:

• the ability to specify a callback for each stage of the detection library, which unlocks
  • real-time capture of quad building or union building issues - we can build a side-by-side comparison
  • enables the building of topics in ROS for detection data or python callbacks which
    • helps aid researchers and students in visualizing and understanding the library
• the ability to build a version of the library without any if(debug) code, asserts, tracing statements (perf)
• the ability to A/B test changes to the mathematics, one of my experiments is in optimizing the matd/matd_op calls and so I need goo diagnostics output to verify changes on different test runs

The example given is specifying an invalid tag family and getting an assertion abort, which even if not the prettiest behavior, doesn't seem to block any specific use of AprilTag. So I would tentatively say that large-scale refactoring like that is perhaps better suited to a fork of the project...

That is a separate PR from the diagnostics feature branch. I was asked to break the diagnostics branch into smaller pieces so I took configurable asserts as the first one. The test output shows the difference between running with and without asserts enabled which can be toggled for both release and debug builds. This is useful so you can 1. build optimized builds with asserts, 2. control the assert behavior on embedded robotics platforms

I haven't been very active looking at issues here, but I agree that I think performance and correctness are the main areas of concern, and so I think changes should be justified in those terms.

Agreed - I actually would say that robustness, correctness, compatibility, and performance in that order is the lens I'm trying to look at. In order to attack some of these problems we need better automated stable tests and some ability to hook into what the library is doing at it's darkest moments :)

[mkrogius]

I think that sounds pretty good to me, and @acshi and I talked offline about this as well.

The main reason I was so worried about the cost of accepting larger PRs is because we do not have any sort of automated testing to validate there will be no regressions. With testing in place I would be much more willing to accept larger refactorings and changes to the library. Also, knowing up front that you are planning to work on this project for the longer-term makes me feel better about investing more of my time into this w/ code reviews and the like.

I'll try to put up a basic regression test this Saturday, and then I'd ask that you contribute improvements to it in the future.

Re: the original question about the makefile, since we are going down the road of making larger changes, I think we should release a version number with the makefile support explicitly deprecated and then plan to bump the major version number when we remove the makefile (and make any other possibly-not-backwards-compatible changes)

[jrepp]

This all sounds very good and I'm happy to collaborate with you and the team on testing before we move forward with any larger changes.

We can close this issue and move forward.

I have some simple CTest framework I can share with you if you want to save some time getting started. I'll start a separate issue with links to that and some things I've found helpful.