-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/openeb updates #123
Changes from all commits
c14641f
ad7c956
736d1d4
1c9c4e4
540052b
951b69d
0d1219c
d97362c
8c030d4
0757f77
05c0498
ee8848a
c286d09
cd47184
c579bf1
1e753c4
19405b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,18 +14,27 @@ | |
|
||
#ifdef _USE_OPENGL_ES3_ | ||
#include <GLES3/gl3.h> | ||
#elif defined(__APPLE__) && !defined(__linux__) | ||
#define GL_SILENCE_DEPRECATION | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily an action request, but do you remember what is the deprecated thing we are using? |
||
#include <OpenGL/gl3.h> | ||
#else | ||
#if defined(WIN32) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe I can change the logic here. I would end up having to write a separate case for linux and including I was able to collapse the first #else to and #elif. |
||
#include <Windows.h> | ||
#endif | ||
#include <GL/glew.h> | ||
#include <GL/gl.h> | ||
#endif | ||
|
||
// GLFW needs to be included after OpenGL | ||
#include <GLFW/glfw3.h> | ||
|
||
// While we keep support for OpenGL, we need to provide a | ||
// dummy implementation for Glew init function | ||
#ifndef GLEW_OK | ||
#define GLEW_OK 0 | ||
inline int glewInit(void) { | ||
return GLEW_OK; | ||
} | ||
#else // OpenGL | ||
#include <GL/glew.h> | ||
#include <GL/gl.h> | ||
#endif | ||
|
||
// GLFW need to be included after OpenGL | ||
#include <GLFW/glfw3.h> | ||
|
||
#endif // METAVISION_SDK_UI_UTILS_OPENGL_API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In regards to this change, I faced two issues while trying to build on Mac.
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!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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