Integration of AprilTag library #950

elektrokokke · 2020-12-11T16:42:34Z

(fix #949).
For now, only the smallest family of tags (tag16h5) is supported,
which consists of 30 coded tags.
For each marker, 5 keypoints are generated:

the center point (computed from the four corner points), with IDs 0-29
the 4 corner points (in counter-clockwise order), with IDs 30-59, 60-89, 90-119, and 120-149
Thus, the feature descriptor has 150 dimensions.

The implementation follows along the lines of the CCTag markers, wherever possible.
In most places where CCTags were used before, AprilTags are now an additional option.
Exceptions are the camera and rig calibration and localization functions,
as well as a function which seems to create visualizations of feature matching results.

The Dockerfile for Centos has been adapted to start an AliceVision build using AprilTags.
I did not dare touch the Dockerfiles for Ubuntu, as the Centos one seemed to be the main one in use,
and it also was the one where the build process went quite smooth.

This has not been tested on Windows, sorry. It seems that AprilTags officially support only Linux,
although Windows should be possible according to the official repo.

There is one change which I do not really like, where it might make sense to change the
underlying classes/interfaces a bit to make different marker feature extractors easier.
This is the change in src/aliceVision/sfm/pipeline/ReconstructionEngine.cpp, which
only works by testing the results of dynamic casts on feature::Regions subclasses, although
the code that follows is rather similar for both CCTags and AprilTags, and could probably
substituted by something more general. The reason for this is that the marker ID is computed
from the descriptor, for which there could be an abstract function in the superclass.
I did not dare to make any such deep changes to the code, though.

(alicevision#949). For now, only the smallest family of tags (tag16h5) is supported, which consists of 30 coded tags. For each marker, 5 keypoints are generated: - the center point (computed from the four corner points), with IDs 0-29 - the 4 corner points (in counter-clockwise order), with IDs 30-59, 60-89, 90-119, and 120-149 Thus, the feature descriptor has 150 dimensions. The implementation follows along the lines of the CCTag markers, whereever possible. In most places where CCTags were used before, AprilTags are now an additional option. Exceptions are the camera and rig calibration and localization functions, as well as a function which seems to create visualizations of feature matching results. The Dockerfile for Centos has been adapted to start an AliceVision build using AprilTags. I did not dare touch the Dockerfiles for Ubuntu, as the Centos one seemed to be the main one in use, and it also was the one where the build process went quite smooth. This has not been tested on Windows, sorry. It seems that AprilTags officially support only Linux, although Windows should be possible according to the official repo. There is one change which I do not really like, where it might make sense to change the underlying classes/interfaces a bit to make different marker feature extractors easier. This is the change in src/aliceVision/sfm/pipeline/ReconstructionEngine.cpp, which only works by testing the results of dynamic casts on feature::Regions subclasses, although the code that follows is rather similar for both CCTags and AprilTags, and could probably substituted by something more general. The reason for this is that the marker ID is computed from the descriptor, for which there could be an abstract function in the superclass. I did not dare to make any such deep changes to the code, though.

elektrokokke · 2020-12-12T00:28:38Z

Ouch, sorry, just pushed another commit to the fork which was meant for a different branch, not realizing that this would go into this pull request.
I reverted that last commit.

simogasp

Hi, thanks for the nice PR!
The major remark is to avoid having the opencv dependency when writing code for apritag as it is not strictly necessary. I think it should not be much work to remove that.
Other than that it makes a very neat addition to the lib!

src/CMakeLists.txt

simogasp · 2020-12-13T18:07:41Z

src/aliceVision/feature/apriltag/ImageDescriber_APRILTAG.cpp

+ImageDescriber_APRILTAG::AprilTagParameters::AprilTagParameters()
+{
+}


Suggested change

ImageDescriber_APRILTAG::AprilTagParameters::AprilTagParameters()

{

}

ImageDescriber_APRILTAG::AprilTagParameters::AprilTagParameters() = default

U can even move it to the hpp

simogasp · 2020-12-13T18:14:08Z

src/aliceVision/feature/apriltag/ImageDescriber_APRILTAG.cpp

+  const cv::Mat graySrc(cv::Size(image.Width(), image.Height()), CV_8UC1, (unsigned char *) image.data(), cv::Mat::AUTO_STEP);
+  // Make an image_u8_t header for the Mat data


I think aprilTag (the library) does not depends on opencv.
Opencv is optional in alicevision, it would be nice keep aprilTag independent from opencv.
Otherwise, even in the cmake u should tie aprilTag to the fact that opencv is ON
Here for example i think u can drop the cv::Mat and use image to fill the image_u8_t

simogasp · 2020-12-13T18:16:05Z

src/aliceVision/feature/apriltag/ImageDescriber_APRILTAG.cpp

+      cv::Point2d tl(det->p[0][0], det->p[0][1]);
+      cv::Point2d bl(det->p[1][0], det->p[1][1]);
+      cv::Point2d br(det->p[2][0], det->p[2][1]);
+      cv::Point2d tr(det->p[3][0], det->p[3][1]);
+      double denominator = ((tl.x-br.x) * (bl.y-tr.y) - (tl.y-br.y) * (bl.x-tr.x));
+      cv::Point2d center(
+        ((tl.x*br.y - tl.y*br.x) * (bl.x-tr.x) - (tl.x-br.x) * (bl.x*tr.y - bl.y*tr.x)) / denominator,
+        ((tl.x*br.y - tl.y*br.x) * (bl.y-tr.y) - (tl.y-br.y) * (bl.x*tr.y - bl.y*tr.x)) / denominator
+      );
+      cv::Point2d points[5] = { center, tl, bl, br, tr };
+      std::size_t indices[5] = { det->id, 30 + det->id, 60 + det->id, 90 + det->id, 120 + det->id};
+      // compute scale from max side length and diagonals (divided by sqare root of 2):


same here, cv:Point2d can be replaced by Vec2, less handy to manage (no .x and .y) but it eases things for the dependencies

simogasp · 2020-12-13T18:18:01Z

src/aliceVision/feature/apriltag/ImageDescriber_APRILTAG.cpp

+      cv::Point2d points[5] = { center, tl, bl, br, tr };
+      std::size_t indices[5] = { det->id, 30 + det->id, 60 + det->id, 90 + det->id, 120 + det->id};
+      // compute scale from max side length and diagonals (divided by sqare root of 2):
+      double scale = std::max({cv::norm(tl-bl), cv::norm(bl-br), cv::norm(br-tr), cv::norm(tr-tl), 0.707*cv::norm(tl-br), 0.707*cv::norm(tr-bl)});


Suggested change

double scale = std::max({cv::norm(tl-bl), cv::norm(bl-br), cv::norm(br-tr), cv::norm(tr-tl), 0.707*cv::norm(tl-br), 0.707*cv::norm(tr-bl)});

const double scale = std::max({cv::norm(tl-bl), cv::norm(bl-br), cv::norm(br-tr), cv::norm(tr-tl), 0.707*cv::norm(tl-br), 0.707*cv::norm(tr-bl)});

simogasp · 2020-12-13T18:27:25Z

src/aliceVision/sfm/pipeline/ReconstructionEngine.cpp

+        const feature::CCTAG_Regions* cctagRegions = dynamic_cast<const feature::CCTAG_Regions*>(&regions);
+        const feature::APRILTAG_Regions* apriltagRegions = dynamic_cast<const feature::APRILTAG_Regions*>(&regions);


@fabiencastan Not sure what happens here, but i have the impression that the 2 dyn cast can work even if the the input regions are not the correct type.
Maybe we should have a "special" region type "markers" from which all the other (cctag, april...) inherit and refactor the code to retrieve the iID?

I changed the existing dynamic cast from reference to pointer. If the input regions do not have the requested type the result will be a null pointer. This is used in the following if statements to execute the piece of code appropriate for the marker type.
I agree that some kind of marker polymorphism might be good once there are multiple types of markers, but I did not dare making any such deep changes together with this otherwise rather shallow (in terms of change of the code base) pull request...

fabiencastan · 2020-12-29T07:58:37Z

AprilTag is not yet integrated in vcpkg, but there is an issue for it:
microsoft/vcpkg#12171

fabiencastan · 2021-02-12T15:20:49Z

@elektrokokke Very sorry for the delay to merge this nice PR.
I would like to merge it but could you remove the commit/revert-commit from the history?

- Removed all usages of OpenCV. - Removed empty constructor definition in favor of "= default" in header. - Changed scale to const.

elektrokokke · 2021-02-26T09:33:17Z

@fabiencastan sorry this took so long. Just removed the two commits as requested.

fabiencastan · 2021-02-26T10:04:09Z

Thanks. Could you also rebase it on the "develop" branch to fix the small conflict?

elektrokokke · 2021-02-26T16:15:57Z

I hope I got this one right...

mkrogius · 2021-05-07T00:09:30Z

Hi, I'm the maintainer of the AprilTag project. I don't know anything about your application, but I would very much recommend using any other tag family than tag16h5. Since it has the fewest bits of any of our tag families, it is the most likely to have false positives. If you have no specific requirements for the choice of a tag family, the default choice should be tagStandard41h12

fabiencastan · 2021-05-07T08:52:50Z

@mkrogius Thanks a lot for the feedback!

As a first step, I will focus on getting this merged (as it's waiting for too long!) and I hope @elektrokokke will have time to look how we can update to other marker types in another PR (which will probably require to add a new way to deal with markers vs descriptors to support more IDs).

simogasp · 2021-05-07T14:40:10Z

Hi, I'm the maintainer of the AprilTag project. I don't know anything about your application, but I would very much recommend using any other tag family than tag16h5. Since it has the fewest bits of any of our tag families, it is the most likely to have false positives. If you have no specific requirements for the choice of a tag family, the default choice should be tagStandard41h12

@mkrogius and @elektrokokke, just for future reference, the only part of code that changes from one type to another is this one?

  apriltag_family_t *tf = tag16h5_create();
  apriltag_detector_t *td = apriltag_detector_create();
  apriltag_detector_add_family(td, tf);

Say we want to switch to tagStandard41h12, we just need to have (I guess) something like apriltag_family_t *tf = tag41h12_create();?
Or do we also need, e.g. change the way the id are encoded in the descriptors?

mkrogius · 2021-05-07T15:16:34Z

tagStandard41h12_create();

^^ Yep, just like that. Although if you've already printed out tags then you'd have to print out stuff in the new tag family

elektrokokke · 2021-05-07T17:47:49Z

Hi! I would say that ideally the way markers are handled would be changed. Right now they are descriptor with a dimensionality corresponding to the number of possible marker ids and only one dimension is non zero for any one marker. It would make sense IMO to encode a marker’s id as just an integer. But for now, in addition to the listed changes one would need to adjust the dimensionality of the descriptor. For larger tag families this quickly gets large, and it’s wasted considering only one integer could do the same. Also computationally it does not scale well, as the current code iterates over one descriptor to get back the marker ID. Marker matching is similarly expensive. Note that the implementation right now produces 5 markers for each tag, one for the center and four for the corners. Cheers, Jarne

…

Hi, I'm the maintainer of the AprilTag project. I don't know anything about your application, but I would very much recommend using any other tag family than tag16h5. Since it has the fewest bits of any of our tag families, it is the most likely to have false positives. If you have no specific requirements for the choice of a tag family, the default choice should be tagStandard41h12 @mkrogius <https://github.com/mkrogius> and @elektrokokke <https://github.com/elektrokokke>, just for future reference, the only part of code that changes from one type to another is this one? apriltag_family_t *tf = tag16h5_create(); apriltag_detector_t *td = apriltag_detector_create(); apriltag_detector_add_family(td, tf); Say we want to switch to tagStandard41h12, we just need to have (I guess) something like apriltag_family_t *tf = tag41h12_create();? Or do we also need, e.g. change the way the id are encoded in the descriptors? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#950 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEHUECROEO7EZW6Q2Y6VFTTMP3V5ANCNFSM4UW4XVXA> .

elektrokokke mentioned this pull request Dec 11, 2020

Integration of AprilTag library according to issue #1179 and AliceVision pull request #950 alicevision/Meshroom#1180

Merged

elektrokokke changed the title ~~[WIP] Integration of AprilTag library according to issue #949~~ Integration of AprilTag library according to issue #949 Dec 11, 2020

natowi added feature type:enhancement labels Dec 11, 2020

simogasp requested changes Dec 13, 2020

View reviewed changes

simogasp changed the title ~~Integration of AprilTag library according to issue #949~~ Integration of AprilTag library Dec 17, 2020

simogasp added the good first contribution label Dec 17, 2020

fabiencastan added this to the 2.4.0 milestone Feb 12, 2021

fabiencastan removed this from the 2.4.0 milestone Feb 12, 2021

According to suggestions of simogasp:

5c3b8cc

- Removed all usages of OpenCV. - Removed empty constructor definition in favor of "= default" in header. - Changed scale to const.

elektrokokke force-pushed the develop branch from 06f8339 to 5c3b8cc Compare February 26, 2021 09:30

Merge branch 'develop' into apriltag16h5

768761d

fabiencastan added this to the 2.5.0 milestone Feb 26, 2021

fabiencastan mentioned this pull request Apr 10, 2021

[cmake] fix include path so that we can include <apriltag/apriltag.h> AprilRobotics/apriltag#140

Merged

[cmake] all-in-one: update apriltag

839b2a6

[cmake] all-in-one: update apriltag commit

7cd1f0e

fabiencastan previously approved these changes Jun 22, 2021

View reviewed changes

Merge branch 'develop' into apriltag16h5

431b354

fabiencastan dismissed their stale review via 431b354 July 23, 2021 07:38

fabiencastan merged commit cb103ac into alicevision:develop Jul 23, 2021

natowi mentioned this pull request Jul 24, 2023

Improve results with LiDAR or Selected Features? alicevision/Meshroom#2122

Open

natowi mentioned this pull request Sep 3, 2023

How to use custom markers? #1494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration of AprilTag library #950

Integration of AprilTag library #950

elektrokokke commented Dec 11, 2020 •

edited by simogasp

Loading

elektrokokke commented Dec 12, 2020 •

edited

Loading

simogasp left a comment

simogasp Dec 13, 2020

simogasp Dec 13, 2020

simogasp Dec 13, 2020

simogasp Dec 13, 2020

simogasp Dec 13, 2020

elektrokokke Dec 14, 2020

fabiencastan commented Dec 29, 2020

fabiencastan commented Feb 12, 2021

elektrokokke commented Feb 26, 2021

fabiencastan commented Feb 26, 2021 •

edited

Loading

elektrokokke commented Feb 26, 2021

mkrogius commented May 7, 2021

fabiencastan commented May 7, 2021

simogasp commented May 7, 2021

mkrogius commented May 7, 2021

elektrokokke commented May 7, 2021 via email •

edited by simogasp

Loading

		const cv::Mat graySrc(cv::Size(image.Width(), image.Height()), CV_8UC1, (unsigned char *) image.data(), cv::Mat::AUTO_STEP);
		// Make an image_u8_t header for the Mat data

	double scale = std::max({cv::norm(tl-bl), cv::norm(bl-br), cv::norm(br-tr), cv::norm(tr-tl), 0.707cv::norm(tl-br), 0.707cv::norm(tr-bl)});
	const double scale = std::max({cv::norm(tl-bl), cv::norm(bl-br), cv::norm(br-tr), cv::norm(tr-tl), 0.707cv::norm(tl-br), 0.707cv::norm(tr-bl)});

		const feature::CCTAG_Regions* cctagRegions = dynamic_cast<const feature::CCTAG_Regions*>(&regions);
		const feature::APRILTAG_Regions* apriltagRegions = dynamic_cast<const feature::APRILTAG_Regions*>(&regions);

Integration of AprilTag library #950

Integration of AprilTag library #950

Conversation

elektrokokke commented Dec 11, 2020 • edited by simogasp Loading

elektrokokke commented Dec 12, 2020 • edited Loading

simogasp left a comment

Choose a reason for hiding this comment

simogasp Dec 13, 2020

Choose a reason for hiding this comment

simogasp Dec 13, 2020

Choose a reason for hiding this comment

simogasp Dec 13, 2020

Choose a reason for hiding this comment

simogasp Dec 13, 2020

Choose a reason for hiding this comment

simogasp Dec 13, 2020

Choose a reason for hiding this comment

elektrokokke Dec 14, 2020

Choose a reason for hiding this comment

fabiencastan commented Dec 29, 2020

fabiencastan commented Feb 12, 2021

elektrokokke commented Feb 26, 2021

fabiencastan commented Feb 26, 2021 • edited Loading

elektrokokke commented Feb 26, 2021

mkrogius commented May 7, 2021

fabiencastan commented May 7, 2021

simogasp commented May 7, 2021

mkrogius commented May 7, 2021

elektrokokke commented May 7, 2021 via email • edited by simogasp Loading

elektrokokke commented Dec 11, 2020 •

edited by simogasp

Loading

elektrokokke commented Dec 12, 2020 •

edited

Loading

fabiencastan commented Feb 26, 2021 •

edited

Loading

elektrokokke commented May 7, 2021 via email •

edited by simogasp

Loading