AprilRobotics / apriltag

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

compatibility with WIN32_LEAN_AND_MEAN #296

Open mitjap opened 8 months ago

mitjap commented 8 months ago

This library is not compatible with WIN32_LEAN_AND_MEAN definition. It fails to find struct timeval which is normally found by windows.h header. But with WIN32_LEAN_AND_MEAN this header is excluded.

We could directly include winsock.h header as it is specified in official documentation (https://learn.microsoft.com/en-us/windows/win32/api/winsock/ns-winsock-timeval).

https://github.com/AprilRobotics/apriltag/blob/b7bd75ca45d62ac431f10b78b69ea86198cbad14/common/time_util.h#L41

I can prepare a PR for you with this change.

christian-rauch commented 8 months ago

What's the advantage of using #define WIN32_LEAN_AND_MEAN? According to https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers:

Define WIN32_LEAN_AND_MEAN to exclude APIs such as Cryptography, DDE, RPC, Shell, and Windows Sockets.

So it makes sense that some included headers would then be missing.

mitjap commented 8 months ago

Excluding rarely used headers reduces namespace pollution and improves build time. If one want to use it this library should not stand in the way if possible. Instead of including a very general header it should include exactly what it needs.

Here is the link to the fix that we use: https://github.com/modriplanetdoo/apriltag/commit/45a0a7069900c82731ffe2e7cca7f2595a13282a

christian-rauch commented 8 months ago

With the diff I better understand what you aim for. I initially understood that you want to use the windows.h header with #define WIN32_LEAN_AND_MEAN and then additionally add the winsock.h header, which would be counterintuitive in my view. Feel free to send a PR with this patch. Also add the required compile flags to the CI in order to make sure that this will not break in future.

Is timeval only directly defined in winsock.h? Is there no specific "time" header that includes this struct?

mitjap commented 8 months ago

As specified in documentation timeval is defined in winsock.h. It it also recommended to use Winsock2.h which is recommended version of this header.

https://learn.microsoft.com/en-us/windows/win32/api/winsock/ns-winsock-timeval Header winsock.h (include Winsock2.h)

I am not aware of any other time-related header.

brmarkus commented 8 months ago

Why is struct timeval (still) used at all might be the question... instead of a plain, standard C or C++ way of dealing with "time"?

christian-rauch commented 8 months ago

Why is struct timeval (still) used at all might be the question... instead of a plain, standard C or C++ way of dealing with "time"?

Do you know of a standard C way of dealing with time at microsecond resolution?

brmarkus commented 8 months ago

A quick pointer could be "https://stackoverflow.com/a/67731965" (https://stackoverflow.com/questions/5833094/get-a-timestamp-in-c-in-microseconds/67731965#67731965).

Haven't grepped AprilTag for where timeval is used... why is a timestamp needed to process AprilTags ;-) ?

christian-rauch commented 8 months ago

A quick pointer could be "https://stackoverflow.com/a/67731965" (https://stackoverflow.com/questions/5833094/get-a-timestamp-in-c-in-microseconds/67731965#67731965).

The response you linked recommends not using timespec_get. So in the end, you are left with the native APIs. The question and the answer are using the POSIX function gettimeofday which is also the one used here.

Haven't grepped AprilTag for where timeval is used... why is a timestamp needed to process AprilTags ;-) ?

The time is used for profiling.