search

AprilRobotics / apriltag

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

Packaging suggestions #256

Open dkogan opened 2 years ago

dkogan commented 2 years ago

Hi. I'm the Debian maintainer. I just released the 3.3.0 release into Debian; it's there now. Ubuntu will pick it up when they make a new release, in a month probably. Thanks for maintaining this library. I have some notes:

Thanks for maintaining this library!

christian-rauch commented 2 years ago
dkogan commented 2 years ago

Hi.

Christian Rauch @.***> writes:

  • Patch 2 makes sense. But I think usually this is done with some macro that switches between OSes and compilers (e.g. support Windows).

This patch is a whitelist of exports. Maybe some compilers (on Windows, say) do this in some other way than an "attribute", but you need SOMETHING tagging each exported function.

  • Patch 5 is already integrated on master.

Cool

  • Patch 6 makes a lot of sense. I was thinking about splitting this before. @dkogan Can you send a PR for this?

It's a lot of typing to do that. Can you grab the patch from the link and "git am whatever.patch"? If not, tell me, and I'll go through the motions.

  • Patch 7 I don't think we should integrate another example executable.

I'd like to suggest this sample to replace the other samples. Not only does it demonstrate the usage of the C API, it allows the library to be utilized without writing any code. For instance, after I apt install apriltag, I can

IMG=/tmp/apriltagrobots_overlay.jpg

apriltag --vnlog $IMG | \
vnl-filter -p xc,id,yc | \
feedgnuplot --image $IMG \
            --domain \
            --dataid \
            --with 'points pt 2 ps 3 lw 5' \
            --square \
            --hardcopy /tmp/tags.pdf

to produce the attached image. This is really convenient. Shipping SOMETHING that can do this into /usr/bin/apriltag would be really nice, even if it isn't my sample.

dkogan commented 2 years ago

tags.pdf

Here's the attachment from the last email. github is dumb.

christian-rauch commented 2 years ago

This patch is a whitelist of exports. Maybe some compilers (on Windows, say) do this in some other way than an "attribute", but you need SOMETHING tagging each exported function.

Yes, there are other OSes and compilers. That is why this has to be placed behind an if-else macro that resolves for different compilers and OSes. The library is supposed to work on multiple systems.

It's a lot of typing to do that. Can you grab the patch from the link and "git am whatever.patch"? If not, tell me, and I'll go through the motions.

You just have to put this into a branch of your GitHub fork and send a PR via the web interface. The idea was this should be reviewed in public here and not just be applied and pushed to upstream.

I'd like to suggest this sample to replace the other samples. Not only does it demonstrate the usage of the C API, it allows the library to be utilized without writing any code.

I don't fully understand what's the motivation behind this. Both current examples demonstrate the C API already. In fact, there is only a C API that is used by the C example, which reads image files from the command line, and the C++ example, which reads from a video stream via OpenCV. I am not sure what the benefit of a third example would be.

If you want to demonstrate the new "apriltag" "core library" without "apriltag-utils", I would suggest modifying the OpenCV example to only do the AprilTag detection without jpeg decoding, option parsing etc. and let OpenCV do the test. You could also modify the base example and show how both libraries ("core" and "utils") are linked at the same time.

christian-rauch commented 2 years ago

@dkogan Can you send some of your patches as individual PRs? Otherwise, I am inclined to just close this for now.

dkogan commented 2 years ago

Hi. Sorry, the github process is way too much of a pain, and it's not necessary. You already have the patches. You can git am the files to whichever branch you like, push it wherever you like, and review it using whatever process you like.

Thanks

j-medland commented 2 years ago

I would be happy to provide some support to get these merged as it aligns with trying to get a vcpkg.io port that builds a library on Windows.

I had to patch in explicit DLL exports for Windows so adding export statements to the public API seems worthwhile - ditto to splitting out the detector from the other functionality.

I'll give it a go and submit a PR when I have something worth looking at.

Cheers

dkogan commented 2 years ago

Thanks, John.

I'd also like to re-state the case for my "sample":

christian-rauch commented 2 years ago

It's a nice thing to ship in /usr/bin/apriltag. For many use cases this alone is sufficient, and it's nice for this project to provide a ready-to-use binary. The Debian packages do this today, and it'd be good if upstream did that too

This repo already has two examples that are ready to use.

Its sources demo the thing that makes users seek out this library, and not the other stuff: nobody uses libapriltag for its option parser. It also doesn't have any heavy dependencies, not even opencv: only libfreeimage to read/write images. I.e. this sample code has a high signal-to-noise ratio, which is good since most users are likely to grab the "apriltag" guts of the code, and throw out everything else

This repo contains a couple of "extras", such as an option parse or jpeg decoder, in order to not rely on external dependencies. No one has to use these extras to use the library. I think most people use external third-party libraries to do optional parsing or image loading anyway. Therefore it makes sense to split those out into a separate library. But it is nice to have an example that does not require too many external libraries.

If we need an example that only links the "core" library without the "extras", then we should reimplement the apriltag_demo with only the "core" library and without external third/party libraries, by e.g. using the PPM image format with an implementation inside the apriltag_demo only.

dkogan commented 2 years ago

It's a nice thing to ship in /usr/bin/apriltag

This repo already has two examples that are ready to use.

This repo does not already deploy a ready-to-use binary, and it would be very nice if it did. Being able to detect apriltags without any code, in the way noted above in this thread would be very nice

OK. You clearly thought about which samples are useful, and I don't need to debate it. Thanks for maintaining this.

christian-rauch commented 2 years ago

This repo does not already deploy a ready-to-use binary

Can you provide details on what is missing in the apriltag_demo to be "ready-to-use"? If you compile and install this project, you will get an apriltag_demo binary that reads and processes images. What kind of test/demo program would be useful for a Debian package?

dkogan commented 2 years ago

Hi Christian. It's been a long time since I looked the the apriltag build system. I just looked again, and you're right: the build system DOES install a binary into /usr/bin. So I guess my complaints are more about what this binary does, and not that it doesn't exist at all.

I think the goal of this binary should be to be usable for finding apriltags, not just to showcase the usage of the library. Today /usr/bin/apriltag_demo doesn't actually output the detections, so it's not useful on its own. It should be able to output the detections, and it should be called "apriltag" not "apriltag_demo".

You can add the extra functionality (output of tags on the console) to apriltag_demo.c if you want to do that. Then you'll get simple_demo.c, but using the apriltag utility libraries, instead of the standard ones.

I think the vnlog format is a good choice for the output: it's just a text table with labelled columns. If you want to do something else, may I ask that you at least keep vnlog as an option?

Also re: third-party libraries. Reducing dependencies and shipping everything yourself is helpful on OSs that still somehow haven't figured out packaging. But it's counterproductive on ones that have. On Debian there's zero cost to doing an 'apt install libfreeimage-dev', and then your application can read all sorts of image formats without a second thought. I get that you want to support OSX, but there's a balance to strike. So /usr/bin/apriltag SHOULD call out to libfreeimage, or something. You can add that via a #if in the sources. Or I can do a distro-patch for Debian specifically.

Thanks.

j-medland commented 1 year ago

Hello

I have been attempting to split the cmake build into two targets - apriltag and apriltag-utils. This works well but when it comes to defining the example builds, the header file includes are difficult to handle if relying on add_target_libraries to work nicely.

Mirroring the header-install directory layout in the project would mean being able to utilize the correct PUBLIC/PRIVATE scope for the include directories.

I am also proposing moving top level .c files to src to make this layout a bit more typical of other projects. Most of the header files would move to include/apriltag as they are required by the utils library.

project/
├── CMake
├── common/
│   ├── double_float_impl.h
│   ├── doubles.h
│   ├── floats.h
│   ├── g2d.c
│   ├── getopt.c
│   ├── ....
│   └── zmaxheap.c
├── example (unchanged)
├── include/
│   └── apriltag/
│       ├── common/
│       │   ├── debug_print.h
│       │   ├── g2d.h
│       │   ├── ...
│       │   └── zmaxheap.h
│       ├── apriltag_math.h
│       ├── apriltag_pose.h
│       ├── apriltag.h
│       ├── tag16h5.h
│       ├── ...
│       └── tagStandard52h13.h
└── src/
    ├── apriltag_pose.c
    ├── apriltag.c
    ├── tag16h5.c
    └── ...

Thoughts?

I can submit a PR with this soon if that is easier.

christian-rauch commented 1 year ago

Moving the files into a subfolder makes sense and I was thinking about this too before. But we do not need to split source and header files necessarily.

We should maybe also split the tag sources into a dedicated subfolder. Maybe it also makes sense to have a dedicated library for the tags? Then there would be a "core", "utils" and "tag" library since users of the apriltag do not necessarily need to use those tags, but can generate their own. We could just name the subfolders accordingly.

One nice thing would be the removal of "GLOB" and the listing of source files directly. Using globing for source files is actually discouraged by CMake, see https://cmake.org/cmake/help/latest/command/file.html#glob:

We do not recommend using GLOB to collect a list of source files from your source tree. If no CMakeLists.txt file changes when a source is added or removed then the generated build system cannot know when to ask CMake to regenerate.

j-medland commented 1 year ago

Thanks for the feedback! I will try and integrate the things you suggest and update when I have some progress