Feature/openeb updates #123

coashby · 2024-06-16T05:20:29Z

Commit Summary

Update to "opt-out" by default in CMake test and Python binding configuration options.
Build support for MacOSX
Updates to Readme and Docs

Notes

Tested Configurations

Platform	OS [Container]	Arch
Apple	MacOS Monterey	M1 pro chipset
Apple	MacOS Monterey [Container - Ubuntu:latest 24.04 LTS]	M1 pro chipset
Linux	Ubuntu 23.10	AMD Ryzen 7 3700X 8-Core
Windows	Window 10	Intel Core i7 7700 4-Core

…lows for build requirements and instructions to be incrementally added.

…sed target two conditions explicitly: 1) Apple platforms NOT running Linux, and 2) platforms running Linux. No special considerations for Windows are made at this time.

…anges allow the apps to be built with a wider range of compatibility in the SW dependency versions maintained among Apple and Linux based package managers.

lbristiel-psee · 2024-06-17T15:45:56Z

Thanks for your contribution @coashby !
We're currently reviewing it and will get back to you shortly.

coashby · 2024-06-17T15:49:21Z

Thanks for your contribution @coashby ! We're currently reviewing it and will get back to you shortly.

Hey of course. You all have a great product and it's helping me get my business started. Hopefully the contributions work on your end and happy to engage in the review process as well.

Cheers

jthierry-psee

Hello,

Thank you for your contribution. I do have a couple of remarks on the CMake/C++ part of things.

jthierry-psee · 2024-06-20T07:10:20Z

CMakeLists.txt

@@ -15,6 +15,9 @@ if(NOT CMAKE_BUILD_TYPE)
        "Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel." FORCE)
 endif(NOT CMAKE_BUILD_TYPE)

+option(COMPILE_PYTHON3_BINDINGS "Enable/Disable Python bindings. Python bindings are disabled by default." OFF)


This option is already defined on line 179 when not building on Android:
https://github.com/prophesee-ai/openeb/blob/main/CMakeLists.txt#L179

Also, is there a reason you set it to off by default? If building python bindings on MacOS is problematic, it can be disabled by default for this platform.

This option is already defined on line 179 when not building on Android

Hey, yeah I didn't catch that definition on line 179.

If building python bindings on MacOS is problematic, it can be disabled by default for this platform.

I am able to build the python bindings on MacOS.

Also, is there a reason you set it to off by default?

The main reason I thought to change this behavior is to present the simplest build case first. As I walked through the documentation trying to understand what was needed and what wasn't needed to build and use the C++ library, I realized gtest, pytest and pybind11 were all optional.
I think by giving users the option to ENABLE features like testing and python bindings empowers them to:

learn more about the product features incrementally

avoid having to install/remove packages that the don't actually need,

I added new documentation. I initially wrote it keeping the options ON be default. I personally found the documentation and build experience better with the options OFF by default so I went with it... but that's just my take.

@lbristiel-psee @ogeorget-psee what do you think?

Hi @coashby, thanks for your contrib and feedbacks !

I agree that anything related to testing the framework should be disabled by default to ease user experience. Even though CMake enable BUILD_TESTING by default, we can enforce to disable it and make sure that gtest and pytest are not required.

Regarding pybind and the python bindinds, it's a different story has they are entirely part of the API and must be provided to our Python end-users. There are legitimate cases where one would want to disable, that's where COMPILE_PYTHON3_BINDINGS should be set to false.

jthierry-psee · 2024-06-20T07:12:20Z

CMakeLists.txt

@@ -15,6 +15,9 @@ if(NOT CMAKE_BUILD_TYPE)
        "Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel." FORCE)
 endif(NOT CMAKE_BUILD_TYPE)

+option(COMPILE_PYTHON3_BINDINGS "Enable/Disable Python bindings. Python bindings are disabled by default." OFF)
+option(BUILD_TESTING "Enable/Disable test applications." OFF)


The BUILD_TESTING variable is already defined when including Ctest:
https://cmake.org/cmake/help/v3.30/module/CTest.html

@jthierry-psee If we want this, we'll need to ensure those are enable in our CI.

jthierry-psee · 2024-06-20T07:25:44Z

sdk/modules/driver/cpp/src/camera_serialization.cpp

@@ -655,7 +655,11 @@ std::ostream &save_device(const Device &d, std::ostream &os) {

    google::protobuf::util::JsonPrintOptions options;
    options.add_whitespace                = true;
+    #if (GOOGLE_PROTOBUF_VERSION >= 5027000)
+    options.always_print_fields_with_no_presence = true;


Do you have any documentation on this?
I don't see this field being part of the JsonPrintOptions:
https://protobuf.dev/reference/cpp/api-docs/google.protobuf.util.json_util/#JsonPrintOptions

I see the field mentionned here:
https://protobuf.dev/news/v26/#JSON
But it states that it is replacing always_print_default_values which is not the field we are using (always_print_primitive_fields)

In regards to this change, I faced two issues while trying to build on Mac.

I couldn't easily come up with a package formula that allowed me to install a compatible version of Protobuf. If I can figure this out, I would remove the code changes and keep everything to < v26.

Once I decided to keep the newer package version, replacing the deprecated function was not straightforward. The best clues I found were from looking at the blame for PrintOptions definition (protobuf/src/google/protobuf/json/json.h) in the latest release version. In particular, these two commits (2699579 and 7d43131 in the protobuf repo.

TBH, I didn't love making this change, since the Protobuf documentation around it wasn't the best IMO. However, I figured it'd start a discussion as how to best resolve. I didn't spend time trying to test the implications of the new struct field, BUT I did test that the version flag worked properly.

Again would love to make this change better. Perhaps just forcing a specific version of protobuf would be best? but that also doesn't seem ideal as time goes on (right?) Just thinking out loud (please don't read anything negative in my comments!)

Sorry for the delay on this topic, it looks like you are right. I opened a issue on protobuf side so they would update the documentation:
protocolbuffers/protobuf#17437

So your change seems ok. However, the renaming of the field seems to have taken place in v26.0 of protobuf, so the version number to check should be 5026000

jthierry-psee · 2024-06-20T07:32:30Z

sdk/modules/ui/cpp/include/metavision/sdk/ui/utils/opengl_api.h

+#include <OpenGL/gl.h>
+#include <OpenGL/glu.h>
+#include <OpenGL/gl3.h>
+#elif defined(__linux__)


We still want to include the below files for windows.

TLDR;

Should GL/gl.h be part of windows? I found this to break on my end.

GL/glew.h needs to be included as well but only for linux and windows. If GL/gl.h should NOT be part of the windows build then I suggest the following replacement logic:

... #if defined(__APPLE__) && !defined(__linux__) #include <OpenGL/gl.h> #include <OpenGL/glu.h> #include <OpenGL/gl3.h> #elif defined(__linux__) #include <GL/glew.h> #include <GL/gl.h> #elif defined(WIN32) #include <GL/glew.h> #endif ...

Yes, I found this area a bit problematic.
On my windows build the installed vcpkg version ofGL/gl.h was causing the build to break.
I then looked at the include files packaged with the Windows installer for the Prophesee SDK. There the GL library does NOT include GL/gl.h. So I decided to try removing it from my build as well and it worked. So that's why I put the preprocessor directives for linux only.

It could be my windows dependencies are not set up correctly?
I wasn't able to get th Metavision::UI lib to build on Windows from the main branch. I ended up having to make some adjustments regardless.

I agree GL/glew.h needs to be included as well. That's why in my cryptic commit message I mentioned no explicit handling for windows (I apologize for that). I figured it would need to be added in, but I also wanted to address the GL/gl.h issue.
I also didn't want three separate #if defined rules if that could be avoided.
I should have just added it since I knew it was needed. Bad call on my part.

I believe my most recent commit addresses most of your OpenGL related concerns.
I was also able to validate a working build on Windows. Thanks for your inputs. Hopefully it's improved and headed in the right direction.

jthierry-psee · 2024-06-20T07:35:45Z

sdk/modules/ui/cpp/src/base_glfw_window.cpp

@@ -46,11 +46,11 @@ struct GLFWContext {
            throw std::runtime_error("Impossible to create a glfw window");

        glfwMakeContextCurrent(window);
-
+#if defined(__linux__)
        if (glewInit() != GLEW_OK) {


Is there a reason this function is not available for MacOS?
Anyway, we still want to call this on windows.

The best way to do what you want is to mimic what we did for GLES:
https://github.com/prophesee-ai/openeb/blob/main/sdk/modules/ui/cpp/include/metavision/sdk/ui/utils/opengl_api.h#L20
And provide a dummy implementation of glewInit() only for MacOS. It will avoid too much #if in actual implementation.

Thanks! This makes a lot more sense now. In the midst of tackling the initial barrage of compiler errors, I missed this implementation hidden under GLES.
I also didn't realize that once I added #include <OpenGL/gl3.h> I didn't need the APPLE specific versions of the OpenGL calls anymore!

Easy changes to make and the MacOS build is still working. Same for the comments below.

sdk/modules/ui/cpp/src/base_window.cpp

…sequently, removing extraneous Apple and Linux preprocessor directives throughout the various metavision::sdk::ui implementation files.

lbristiel-psee · 2024-06-21T07:06:09Z

@coashby I let @jthierry-psee discuss the programming aspects of this PR with you. On my side, I plan to review your proposition around documentation and will see what/how to integrate them.

coashby · 2024-06-21T07:46:08Z

@coashby I let @jthierry-psee discuss the programming aspects of this PR with you. On my side, I plan to review your proposition around documentation and will see what/how to integrate them.

Ok, cool. Thank you! I appreciate your consideration and look forward to your feedback.

jthierry-psee · 2024-07-12T08:25:36Z

sdk/modules/ui/cpp/include/metavision/sdk/ui/utils/opengl_api.h

 #else // OpenGL
+#if defined(__APPLE__) && !defined(__linux__)


I'm not very familiar with MacOS environments, are there cases where both __APPLE__ and __linux__ are defined?

Yes, I came across this issue during development. This helps anyone that is doing containerized development work. It allows the intended code paths when doing local development of linux containers on Apple silicon.

jthierry-psee · 2024-07-12T08:28:11Z

sdk/modules/ui/cpp/include/metavision/sdk/ui/utils/opengl_api.h

 #else // OpenGL
+#if defined(__APPLE__) && !defined(__linux__)
+#define GL_SILENCE_DEPRECATION


Not necessarily an action request, but do you remember what is the deprecated thing we are using?

jthierry-psee · 2024-07-12T08:28:30Z

sdk/modules/ui/cpp/include/metavision/sdk/ui/utils/opengl_api.h

+#define GL_SILENCE_DEPRECATION
+#include <OpenGL/gl3.h>
+#else
+#if defined(WIN32)


Nit: #elif

I don't believe I can change the logic here. I would end up having to write a separate case for linux and including #include <GL/glew.h> and #include <GL/gl.h> in both logic branches for WIN32 and linux.

I was able to collapse the first #else to and #elif.

jthierry-psee · 2024-07-12T08:30:41Z

@coashby Sorry for the delay on this review. Most of the changes on the code look ok. Just a few remarks/questions on the GL code.
Also, not convinced BUILD_TESTING should be redefined you our CMakeLists.txt

coashby · 2024-07-12T10:08:27Z

@jthierry-psee No worries. Thanks for the most recent review. I agree with your suggestions and I should be able to resolve them by monday of next week.

As far as the OpenGL deprecation note, I believe most of the issues are related to OSX. Apple has discontinued support for OpenGL in their Mojave 10.14 updated. This just ignores all the warnings that pop up in response.

jthierry-psee · 2024-07-12T10:12:21Z

@jthierry-psee No worries. Thanks for the most recent review. I agree with your suggestions and I should be able to resolve them by monday of next week.

Great, thanks. I'll be quicker for the next review/approval.

As far as the OpenGL deprecation note, I believe most of the issues are related to OSX. Apple has discontinued support for OpenGL in their Mojave 10.14 updated. This just ignores all the warnings that pop up in response.

Thanks for the info. If you can add just a small comment about this so we remember why it's here, it would be great.

…ming of takes place in v26.0 and later.

…E_DEPRECATION directive is used to supress warninngs on OSX platforms related to the deprecation of OpenGL on Apple silicon. On MacOS this began with update 10.14.

ogeorget-psee · 2024-07-15T16:26:49Z

CMakeLists.txt

@@ -15,6 +15,9 @@ if(NOT CMAKE_BUILD_TYPE)
        "Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel." FORCE)
 endif(NOT CMAKE_BUILD_TYPE)

+option(COMPILE_PYTHON3_BINDINGS "Enable/Disable Python bindings. Python bindings are disabled by default." OFF)


Hi @coashby, thanks for your contrib and feedbacks !

I agree that anything related to testing the framework should be disabled by default to ease user experience. Even though CMake enable BUILD_TESTING by default, we can enforce to disable it and make sure that gtest and pytest are not required.

Regarding pybind and the python bindinds, it's a different story has they are entirely part of the API and must be provided to our Python end-users. There are legitimate cases where one would want to disable, that's where COMPILE_PYTHON3_BINDINGS should be set to false.

ogeorget-psee · 2024-07-15T16:39:09Z

CMakeLists.txt

@@ -15,6 +15,9 @@ if(NOT CMAKE_BUILD_TYPE)
        "Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel." FORCE)
 endif(NOT CMAKE_BUILD_TYPE)

+option(COMPILE_PYTHON3_BINDINGS "Enable/Disable Python bindings. Python bindings are disabled by default." OFF)
+option(BUILD_TESTING "Enable/Disable test applications." OFF)


@jthierry-psee If we want this, we'll need to ensure those are enable in our CI.

ogeorget-psee · 2024-07-15T16:45:57Z

CMakeLists.txt

@@ -15,6 +15,9 @@ if(NOT CMAKE_BUILD_TYPE)
        "Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel." FORCE)
 endif(NOT CMAKE_BUILD_TYPE)

+option(COMPILE_PYTHON3_BINDINGS "Enable/Disable Python bindings. Python bindings are disabled by default." OFF)
+option(BUILD_TESTING "Enable/Disable test applications." OFF)


Suggested change

option(BUILD_TESTING "Enable/Disable test applications." OFF)

option(BUILD_TESTING "Build OpenEB's test suite" OFF)

Thank you @ogeorget-psee for the clarifications.
@jthierry-psee , Yes I agree tremendously. I know changing the default behavior would have down steam affects, so I understand and appreciate that these decisions are not taken lightly.

I'll update the docs to include pybindings as part of the required build components.

I'll also work to resync this branch with main.

@ogeorget-psee, @jthierry-psee, finished making those changes.
Obviously it'll take some time to review but always grateful for the feedback once you have time.

Cheers.

CMakeLists.txt

Updating option comment for BUILD_TESTING Co-authored-by: Olivier Georget <116826255+ogeorget-psee@users.noreply.github.com>

…. Python binding are enabled by default to satisfy API requirements. The default ON behavior is defined for all NON-Android platforms. Documentation has also been updated to reflect this behavior.

lbristiel-psee · 2024-08-23T09:10:45Z

@coashby coming back to you about this PR, sorry for the delay.
Thanks again for the contribution!
Let's try to get as much possible merged into the repo.
The modifications/additions you have done on the README and docs folder are a bit complicated for us to merge for multiple reasons (we don't want to document the Mac install as we don't have the resources to test/maintain/support this platform, we are focussing on improving the Open Install in our SDK online doc rather than the README etc.).
So in order for us to merge this PR, could you remove the documentation part (README and "docs" folder)?
Thanks in advance

coashby · 2024-08-23T10:10:02Z

@coashby coming back to you about this PR, sorry for the delay. Thanks again for the contribution! Let's try to get as much possible merged into the repo. The modifications/additions you have done on the README and docs folder are a bit complicated for us to merge for multiple reasons (we don't want to document the Mac install as we don't have the resources to test/maintain/support this platform, we are focussing on improving the Open Install in our SDK online doc rather than the README etc.). So in order for us to merge this PR, could you remove the documentation part (README and "docs" folder)? Thanks in advance

Sure, I completely understand. I'll try and get that done today or over the weekend.

Regards

…see's end and documenting the process introduces added complexity for both maintainers and end-users.

coashby · 2024-10-01T19:05:29Z

Sorry for the long delay on my end as well. I forgot what happened that week, but clearly I did not get back to this as I planned (T_T).
I removed the docs folder and updated the readme to its original contents. LMK if everything appears good on your end!
Cheers

jthierry-psee · 2024-10-09T14:26:16Z

@coashby finally merged it. Sorry it took a bit of time.
Thanks again for your contribution!

coashby · 2024-10-09T15:58:36Z

@coashby finally merged it. Sorry it took a bit of time. Thanks again for your contribution!

No worries. That's how it goes with community contributions sometimes. Grateful for the interactions and feedback.
The SDK has been helpful for my company so happy to give back what I can.

Cheers

coashby added 6 commits June 15, 2024 21:29

Enabling a default setting for the cmake build configuration. This al…

c14641f

…lows for build requirements and instructions to be incrementally added.

Enabling Apple specific OpenGL support. The preprocessor directives u…

ad7c956

…sed target two conditions explicitly: 1) Apple platforms NOT running Linux, and 2) platforms running Linux. No special considerations for Windows are made at this time.

Updating SDK apps to use updated Boost and Protobuf methods. These ch…

736d1d4

…anges allow the apps to be built with a wider range of compatibility in the SW dependency versions maintained among Apple and Linux based package managers.

Updating repo documentation.

1c9c4e4

Syncing fork.

540052b

Typo correction. Renaming docs build guides.

951b69d

coashby added 3 commits June 17, 2024 12:28

Fixing broken link in README.

0d1219c

Fixing merge.

d97362c

Removing references to emsight from build tutorials..

8c030d4

jthierry-psee reviewed Jun 20, 2024

View reviewed changes

Fixing OpenGL logic for supporting Linux/Windows/Apple platforms. Sub…

0757f77

…sequently, removing extraneous Apple and Linux preprocessor directives throughout the various metavision::sdk::ui implementation files.

E1k3 mentioned this pull request Jul 6, 2024

protobuf::json::PrintOptions.always_print_primitive_field was deprecated and was removed from the current release #126

Closed

jthierry-psee reviewed Jul 12, 2024

View reviewed changes

coashby added 2 commits July 13, 2024 19:31

Updating field name change #if to protobuf version to v26.0. The rena…

05c0498

…ming of takes place in v26.0 and later.

Minor changes to the preprocessor logic. Also note that the GL_SILENC…

ee8848a

…E_DEPRECATION directive is used to supress warninngs on OSX platforms related to the deprecation of OpenGL on Apple silicon. On MacOS this began with update 10.14.

ogeorget-psee reviewed Jul 15, 2024

View reviewed changes

coashby and others added 3 commits July 15, 2024 21:59

Update CMakeLists.txt

c286d09

Updating option comment for BUILD_TESTING Co-authored-by: Olivier Georget <116826255+ogeorget-psee@users.noreply.github.com>

Removing the default OFF behavior for COMPILE_PYTHON3_BINDINGS option…

cd47184

…. Python binding are enabled by default to satisfy API requirements. The default ON behavior is defined for all NON-Android platforms. Documentation has also been updated to reflect this behavior.

Merging Release v4.6.2 changes (SHA: 53b3618~).

c579bf1

coashby added 2 commits October 1, 2024 14:44

Removing docs modifications. MacOS installation is untested on Prophe…

1e753c4

…see's end and documenting the process introduces added complexity for both maintainers and end-users.

Updating the README to the correct version.

19405b8

jthierry-psee approved these changes Oct 9, 2024

View reviewed changes

jthierry-psee merged commit b93c18f into prophesee-ai:main Oct 9, 2024
3 of 6 checks passed

coashby deleted the feature/openeb-updates branch October 9, 2024 16:45

		#else // OpenGL
		#if defined(__APPLE__) && !defined(__linux__)

	option(BUILD_TESTING "Enable/Disable test applications." OFF)
	option(BUILD_TESTING "Build OpenEB's test suite" OFF)

Feature/openeb updates #123

Feature/openeb updates #123

Conversation

coashby commented Jun 16, 2024

Commit Summary

Notes

Tested Configurations

lbristiel-psee commented Jun 17, 2024

coashby commented Jun 17, 2024

jthierry-psee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coashby Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbristiel-psee commented Jun 21, 2024

coashby commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jthierry-psee commented Jul 12, 2024

coashby commented Jul 12, 2024

jthierry-psee commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbristiel-psee commented Aug 23, 2024

coashby commented Aug 23, 2024

coashby commented Oct 1, 2024

jthierry-psee commented Oct 9, 2024

coashby commented Oct 9, 2024

coashby Jun 20, 2024 •

edited

Loading