esmini icon indicating copy to clipboard operation
esmini copied to clipboard

Replaced function pointer callbacks with function objects

Open LukasWikander opened this issue 2 years ago • 3 comments

Replaced c-style function pointers for StoryBoardChange in esminiLib.cpp with std::function to avoid forcing static methods on users of the API. Amended the external API with a function for accepting std::functions alongside the current function pointer signature.

If this is accepted as a good idea I aim to do the same for the other SE_RegisterXXXCallbacks.

LukasWikander avatar Feb 07 '23 12:02 LukasWikander

First glance this looks great, thanks for contributing! Also nice that it passes the recently sharpened CI builds and tests 👍

However, before merging I need better understanding.

Background (to the esminiLib): The ambition has been from start to make esmini as portable as possible. Not only should esmini standalone tools work on "all" systems (Win, Linux, Mac). But the library should integrate easily with all kinds of platforms, e.g. Unity (plugin with C# interface), MATLAB/Simulink (as C-function), web (java bindings), Android (not yet explored)... So to make this feasible, and with our limited insight of potential pitfalls, we decided to go for as pure ANSI-C as possible for the lib API.

That explains the sometimes clumsy patterns of not passing vectors or other non primitive types, instead first checking for number of items and then loop to fetch the individual information (for example: get position of all objects).

Based on above, I suspect that the std::function could be a problem for some primitive environments, expecting header file to be in pure C....? But I'm on thin ice here, maybe we already broke that C-only policy, or maybe this will work just as fine with pure C interface? What do you think?

Unfortunately we don't have worst-case tests in this respect part of the CI - that's why I'm a bit careful.

Can you elaborate on the problem, I guess you encountered an issue related to static functions?

eknabevcc avatar Feb 08 '23 09:02 eknabevcc

I suspected that you were aiming for C-only. OSCAction.hpp already contained some other C++ code so that one will at least not become less C compatible. Same goes for esminiLib.cpp and OSCAction.cpp. The esminiLib.hpp header however only contained C compatible code. There, I surrounded the new C++ code with #ifdef __cplusplus to avoid becoming non-compatible and did not remove the C-style endpoint, so there should not be any difference for C code using the interface. Both the existing C endpoint and C++ endpoint call the same underlying code in esminiLib.cpp.

LukasWikander avatar Feb 22 '23 08:02 LukasWikander

Thanks. I will review and look closer asap.

Meanwhile a clarification: esmini is definitely based on C++, all modules. Also the esminiLib.cpp. Everything will be compiled into binary shared library. When this library is being used in any other software, e.g. Simulink or Unity, the original code language does not matter any more. But the interface should be pure C. That's the reason why esminiLib.hpp is kept primitive C.

eknabevcc avatar Feb 22 '23 09:02 eknabevcc