search

chili-epfl / chilitags

Robust Fiducial Markers for Augmented Reality And Robotics
http://chili.epfl.ch/software
123 stars 57 forks source link

Expose the camera field of view in Chilitags3D #61

Closed severin-lemaignan closed 6 months ago

severin-lemaignan commented 9 years ago

Useful in particular to rebuild projection matrices in OpenGL for instance.

severin-lemaignan commented 9 years ago

@ayberkozgur : thanks for the review. I've updated the PR with the vertical FOV instead of the horizontal one.

qbonnard commented 9 years ago

How about adding a little test ? Something minimal would do, just so we stop adding untested code, even trivial...

qbonnard commented 9 years ago

Then the usual question (you're usually the one asking it, so here is your karma): is this functionality worth the "increased complexity of the api" ? To be the devil advocate: if you don't use raw OpenGL, your framework most likely has a utility to build a projection matrix for you... If not, well, you're not gonna be bothered by one more line to type yourself ;)

ayberkozgur commented 9 years ago

I think one float getter hardly increases the complexity of an API :)

qbonnard commented 9 years ago

You mean one [method with whose name is an acronym, making non obvious computations on double matrices, and returning a] float, right ? Looks like a boolean coding for horizontal/vertical is complex enough to require a code review ;)

One method is about 10% of the current 3D API. How about a comment in the documentation of getCameraMatrix giving the formula to compute the FOV, as an illustration of what's in the camera matrix?

severin-lemaignan commented 9 years ago

@qbonnard I almost agree with you... the OpenGL tools I know all take the vertical FOV to setup the camera. So it may be convenient (especially since the FOV can be updated in several ways) to have it there anyway... not sure.

ayberkozgur commented 9 years ago

@qbonnard You're trying to convince the wrong guy, I'm OK with as many API components as needed as long as they are all properly documented :)

The fact that FOV is not something "provided" by Chilitags but is already there is another thing though. The user, in their framework, provides the FOV (and other calibration parameters) to Chilitags and should "know about" them when they're necessary; it should not ask them from Chilitags.

qbonnard commented 9 years ago

Okay, let's flip side again ;) Following @ayberkozgur 's argument, we should also remove getCamera then, since the user should also know about it... The use case was for calibrations read from a file given to Chilitags, to avoid parsing it again by the user.

Now I'm lost about who thinks what, starting with myself. But I'm happy to see that @severin-lemaignan is a Unity evangelist ;)

Anyway, I wouldn't store the FOV, but rather recompute it on each call to getFOV, because I don't expect the user to query the FOV every frame (and even then, it's no drama), but it removes 4 lines (out of 5 ;) ). Also, you slipped @bhack 's change of the ITERATIVE parameter into your commit, if I'm not wrong... Is that on purpose ?

severin-lemaignan commented 9 years ago

@qbonnard Ok, replaced 5 lines with a single line + removed the SOLVE_ITERATIVE that actually slipped there.

qbonnard commented 9 years ago

With a little test, it would be perfect ;) Actually, that would be a good way to evaluate whether a change to the API is worth it: "Is the commiter ready to provide unit tests for it ?" So far, only Filter has some kind of unit test. How about Starting a Chilitags3D unit test (even if it has only getFOV in it for now)...