chili-epfl / chilitags

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

Concave quad screening #79

Closed ayberkozgur closed 9 years ago

ayberkozgur commented 9 years ago

Solves issue #71 and also provides a place where we can put fast and simple code to screen out stuff that are obviously not tags.

severin-lemaignan commented 9 years ago

What about making isConvex static so that we do not have to instantiate one more class? would it 'break the design'?

ayberkozgur commented 9 years ago

The ScreenOut object is stateless and by design we should strive to keep it stateless in the future. Once we start to keep the history of tags, it stops being a fast & simple piece of code. Therefore making it static would not affect the design.

qbonnard commented 9 years ago

About breaking the design, I think it applies to a few other Classes-formerly-known-as-pipeables (I'm thinking about Séverin's beloved EnsureGreyscale for example). The Classes-formerly-known-as-pipeables pretend to be kinda functional, so statelessness is something we want. Sometimes it is not possible, and sometimes the state is just a performance optimisation (typically, for not reallocating cv::Mat every frame).

I find it esthetically disturbing to break the uniformity of the calls (mFunctor() versus SomeClass::someFunction()) but it's all internal, so if I really make nightmares about it, I can always refactor ;)

Actually, you don't need the class anymore, do you ? Then we could go for if(isConvex(quad)) trackedTags[tag.first] = quad;

(And while you're at it, #include <map> and #include <opencv2/core/core.hpp> are useless if I'm not wrong)

Right, enough complaining. I'll try it live now ;)

qbonnard commented 9 years ago

My 5.46 seconds of trying the sample make me happy. Another cool contribution, thanks ! I can merge if you want (and if you're ok with removing the class and the includes)

ayberkozgur commented 9 years ago

I'm happier with isConvex as a member of something. Otherwise it looks too much like an unorderly c code.

qbonnard commented 9 years ago

Then it can be the member of a namespace, otherwise it looks too much like unorderly Java code ;) But it's actually already a member of the chilitags namespace...

ayberkozgur commented 9 years ago

I like java namespacing a lot and find it very orderly (apart from very long names) like explicit enum naming and "everything must be in a class".

I might look as if I'm a jeverything hater but I'm very much not :)

qbonnard commented 9 years ago

I like explicit too, that's why I never use namespace anything;

My problem with the class as namespace is semantical: class A {int member; static int doSomething(); }; means that all instances of A have a member that is an integer, and in A there is a function to doSomething. So class A {static int doSomething(); }; means: "all instances of A, and in A there is a function to doSomething". Java does not have a namespace notion, which only says in A there is a function to do Something.

I might look like I'm an obsessive hair splitter, but I am ;)

But sometimes, I manage to get ahold of myself, so I merged your cool PR, thanks again !

ayberkozgur commented 9 years ago

That's why there will be no members or instances of A i.e ScreenOut as long as we can manage to keep it stateless :) Just for the record, we could have used namespace ScreenOut but class ScreenOut is parallel with the rest of the functionality grouped in classes, and we can extend it to be non-static if we ever need to make it stateful.

ayberkozgur commented 9 years ago

By the way, Java's packages is probably closest to namespaces in terms of functionality, and is another feature I find highly beneficial: Probably less powerful but also does less harm than c++'s namespaces.