Add opengl context and X11 dependency for use in render engine gl #12862
Conversation
|
+@SeanCurtis-TRI for review please |
There was a problem hiding this comment.
+@sherm1 for platform review, please.
Reviewed 3 of 3 files at r1.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)
There was a problem hiding this comment.
I can't adequately platform review this. I tried to compare the proposed files with existing tools/workspace repositories but each one looks different and none that I saw looked exactly like these. I saw Mac-specific code in some of the other files but none here, though I understand we will only include the opengl renderer on Ubuntu. I'm happy to have x11 included on both platforms regardless, but want to make sure that's the intent.
I've got to punt to Jeremy though to make sure this gets a competent platform review.
-@sherm1 +@jwnimmer-tri
Reviewed 3 of 3 files at r1.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @tehbelinda)
a discussion (no related file):
This PR actually has no effect (other than linting), because nothing depends on the new @x11, and bazel only pulls dependencies on demand. To ensure CI coverage, we need (1) for some cc_library to deps = [] on the new repository and (2) for some C++ code to #include one of the relevant headers from this new repository. Those would prove that everything is wired up correctly.
a discussion (no related file):
See tools/install/libdrake/BUILD.bazel at name = "libdrake_runtime_so_deps". Since this introduces a new runtime dependency on libX11 (because pydrake has gl renderer bindings), we will need to add @x11 into that list, but only on Ubuntu, I think? Follow the pattern of :nlopt_deps in that same BUILD file, but the condition on the select should be the macos/ubuntu one. So :x11_deps with a select and libdrake_runtime_so_deps lists :x11_deps.
a discussion (no related file):
The libX11.so file which this PR requires comes from libx11-dev in Ubuntu, but neither of drake/setup/ubuntu/{source|binary}_distribution/packages-bionic.txt list that package. Is it implied by one of the other items already listed here? We may still want to add it explicitly anyway, for consistency. We'd put libx11-dev into source, and libx11-6 into binary.
tools/workspace/default.bzl, line 85 at r1 (raw file):
load("@drake//tools/workspace/uritemplate_py:repository.bzl", "uritemplate_py_repository") # noqa load("@drake//tools/workspace/vtk:repository.bzl", "vtk_repository") load("//tools/workspace/x11:repository.bzl", "x11_repository")
nit By convention, we place @drake at the start of all of these lines, so be consistent with this new one.
tools/workspace/x11/repository.bzl, line 9 at r1 (raw file):
) def x11_repository(
This dependency should only appear on Ubuntu, I believe? In which case, we need an OS conditional here. See tools/workspace/libblas for a relevant example.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @tehbelinda)
a discussion (no related file):
Also, I guess I'm not 100% sure where in #12788 this new library is used? I didn't immediately find any direct use. If its an indirect use, it might be better to add our direct dependency instead.
Could you point me to the planned use in our C++ code, and I can try to chase down what's needed for the build?
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @tehbelinda)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Also, I guess I'm not 100% sure where in #12788 this new library is used? I didn't immediately find any direct use. If its an indirect use, it might be better to add our direct dependency instead.
Could you point me to the planned use in our C++ code, and I can try to chase down what's needed for the build?
It's the opengl_context.h/cc file that makes use of the x windows APIs.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @SeanCurtis-TRI and @tehbelinda)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
It's the
opengl_context.h/ccfile that makes use of the x windows APIs.
Hmm, still struggling. Can you help me out and point specifically to one include statement and one called function name that are coming from libX11?
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @tehbelinda)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Hmm, still struggling. Can you help me out and point specifically to one include statement and one called function name that are coming from libX11?
The inclusion comes by way of
Usages include every function starting with glX.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @tehbelinda)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
The inclusion comes by way of
Usages include every function starting with glX.
There's even a comment indicating that the True value comes from xlib.h.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @SeanCurtis-TRI and @tehbelinda)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
There's even a comment indicating that the
Truevalue comes from xlib.h.
Hm. GLX is beyond what libX11 provides, so this PR is probably incomplete and/or misnamed. Since the amount of code here is relatively tiny, and since we anyway need to add a proof-of-life uses to this PR before merging anyway, would it make more sense to roll this change back into the opengl_context PR once that is broken out of the mega PR?
Alternatively, we could develop a small glx proof of life unit test and add it in here -- which might be useful as a smaller-than-renderer diagnostic aid as we spin this up? Or if that toy program is anyway as big as our opengl_context wrapper in the first place, we might as well just stick with the class we already have. It seems pretty small, so maybe just iterating on that one class and its unit test makes sense -- and closing this PR.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @BEL and @tehbelinda)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Hm. GLX is beyond what libX11 provides, so this PR is probably incomplete and/or misnamed. Since the amount of code here is relatively tiny, and since we anyway need to add a proof-of-life uses to this PR before merging anyway, would it make more sense to roll this change back into the opengl_context PR once that is broken out of the mega PR?
Alternatively, we could develop a small glx proof of life unit test and add it in here -- which might be useful as a smaller-than-renderer diagnostic aid as we spin this up? Or if that toy program is anyway as big as our opengl_context wrapper in the first place, we might as well just stick with the class we already have. It seems pretty small, so maybe just iterating on that one class and its unit test makes sense -- and closing this PR.
What you're saying makes perfect sense. Perhaps this was the wrong slice to extract from the monster port PR. Either way, an existence proof that this satisfies its need would go a long way.
@BEL How about the following. Add the opengl-context to this PR and have a test that attempts to instantiate an OpenGL context?
|
a discussion (no related file): Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Sure, I'll try that out first, I'd rather that than add back to the monster PR 😅 |
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @tehbelinda)
a discussion (no related file):
Previously, tehbelinda (Bel) wrote…
Sure, I'll try that out first, I'd rather that than add back to the monster PR 😅
OK Sure -- morphing this PR into opengl_context SGTM. I'll watch for any updates.
tehbelinda
force-pushed
the
render-engine-gl-x11
branch
2 times, most recently
from
4d1bc50
to
defd39a
Compare
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri, @SeanCurtis-TRI, and @sherm1)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This PR actually has no effect (other than linting), because nothing depends on the new
@x11, and bazel only pulls dependencies on demand. To ensure CI coverage, we need (1) for some cc_library todeps = []on the new repository and (2) for some C++ code to#includeone of the relevant headers from this new repository. Those would prove that everything is wired up correctly.
Done.
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
See
tools/install/libdrake/BUILD.bazelatname = "libdrake_runtime_so_deps". Since this introduces a new runtime dependency on libX11 (because pydrake has gl renderer bindings), we will need to add@x11into that list, but only on Ubuntu, I think? Follow the pattern of:nlopt_depsin that same BUILD file, but the condition on the select should be the macos/ubuntu one. So:x11_depswith a select andlibdrake_runtime_so_depslists:x11_deps.
Done.
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The
libX11.sofile which this PR requires comes fromlibx11-devin Ubuntu, but neither ofdrake/setup/ubuntu/{source|binary}_distribution/packages-bionic.txtlist that package. Is it implied by one of the other items already listed here? We may still want to add it explicitly anyway, for consistency. We'd putlibx11-devinto source, andlibx11-6into binary.
Done.
tools/workspace/x11/repository.bzl, line 9 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This dependency should only appear on Ubuntu, I believe? In which case, we need an OS conditional here. See tools/workspace/libblas for a relevant example.
Done.
jwnimmer-tri
changed the title
There was a problem hiding this comment.
I think the build system part of this PR still had a problem where we're using glx but nothing in the workspace is depending on / checking for that. However, I think I should hold off dealing with that until the CI question is resolved, to make sure we're solid on what APIs are actually required before we make sure the build system access to those APIs is precisely phrased.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri, @SeanCurtis-TRI, and @sherm1)
tehbelinda
force-pushed
the
render-engine-gl-x11
branch
from
defd39a
to
a8c5e33
Compare
tehbelinda
force-pushed
the
render-engine-gl-x11
branch
2 times, most recently
from
74d4027
to
deebd1b
Compare
|
@drake-jenkins-bot linux-bionic-unprovisioned-gcc-bazel-experimental-release please |
|
@drake-jenkins-bot mac-catalina-unprovisioned-clang-bazel-experimental-release please |
tehbelinda
force-pushed
the
render-engine-gl-x11
branch
from
deebd1b
to
d8eee0b
Compare
|
@drake-jenkins-bot linux-bionic-unprovisioned-gcc-bazel-experimental-release please |
tehbelinda
changed the title
There was a problem hiding this comment.
Redone back to X11 plus some updates from jeremy's comments
+@SeanCurtis-TRI can you re-feature review please?
Reviewable status: 15 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform), labeled "do not merge" (waiting on @jwnimmer-tri, @SeanCurtis-TRI, @sherm1, and @tehbelinda)
geometry/render/gl_renderer/BUILD.bazel, line 42 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Hmm. Do we directly use opengl from these files? Or is it only indirectly used via glew and/or glut?
If the latter, the deps on opengl should be in package.BUILD.bazel under workspace/... for the one or both of glew and glut that need opengl, and we should only deps-what-we-use here (omit opengl).
should no longer be relevant now that it's back to X
geometry/render/gl_renderer/opengl_context.h, line 10 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The namespace
glis not valid per GSG because it is not congruent with the directory name(s). " The code in that namespace should usually be in a directory whose basename matches the namespace name (or in subdirectories thereof)."The directory could be shortened to
gl, or the namespace could be expanded togl_renderer. It would also be valid to remove the fourth-nested namespace entirely and place this all withinrender. In general, for platform review, the namespaces need to be a subset of the dirnames, not an exact match.
Ok getting rid of gl and using the internal namespace instead
geometry/render/gl_renderer/opengl_context.h, line 17 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Should this class be inside an
internalnamespace? My impression was thatMakeRenderEngineGlwas going to be the sole public API for this entire component.
Done.
geometry/render/gl_renderer/opengl_context.h, line 27 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I do not understand what this means. Presumably "context" is shorthand for "GL context" (the thing our constructor initialized). But why should we want to release it or not, and/or what happens if it was not owned? And what does "owned" mean in the first place?
Ah this was leftover from the port, I've removed references to ownership now, thanks
geometry/render/gl_renderer/opengl_context.h, line 33 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This always returns
true. Is there some future plan where it will not? If no, remove it. If yes, explain in an API comment when it could be false and what to do about it if so.
Done.
geometry/render/gl_renderer/opengl_context.h, line 44 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Reference to
glx.hseems stale (x2).
Updated back to X now!
geometry/render/gl_renderer/opengl_context.cc, line 10 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit unused
drake_throw.
It's used once again now
geometry/render/gl_renderer/opengl_context.cc, line 22 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
It seems wrong to print debug messages into the ERROR log channel (for type != ERROR, at least)?
Done.
geometry/render/gl_renderer/opengl_context.cc, line 43 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
TBD Reminder for myself to check if this flashes a window during unit testing.
should no longer be relevant now that it's back to X
geometry/render/gl_renderer/opengl_context.cc, line 57 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The fmtlib/fmt#73 suggests that this should work without the cast. Does it?
Yep!
geometry/render/gl_renderer/opengl_context.cc, line 64 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Unclear to me when this is ever zero.
should no longer be relevant now that it's back to X
geometry/render/gl_renderer/opengl_context.cc, line 93 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This check is racy (not threadsafe). The best way to make it safe is with a thread local static -- which the compiler guarantees is initialized precisely once upon first use -- that is also a POD (so has no destructor).
// Ensure that glutInit and friends have been called exactly once per process upon return. static void LatchInitializeGlut() { static int dummy = InitializeGlut(); unused(dummy); } // Do not call this directly, call LatchInitializeGlut instead. static int InitializeGlut() { // ... glutInit(/* ... */); // ... return 0; // ignored dummy value }
should no longer be relevant now that it's back to X
geometry/render/gl_renderer/opengl_context.cc, line 97 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Per https://www.opengl.org/resources/libraries/glut/spec3/node9.html yes, this is exactly once per process.
Also https://www.opengl.org/resources/libraries/glut/spec3/node10.html says that
glutInitwill terminate the program on error. We'd really much prefer to use exceptions instead of termination -- we don't really want to take down the python interpreter if DISPLAY was not set correctly.The philosophy in https://www.opengl.org/resources/libraries/glut/spec3/node4.html also sounds incompatible with Drake as a library.
Is there a lighter-weight way to get what we need? The above docs make me wonder if glut is suitable for us.
As per Slack discussion, reverting back to X and avoiding GLUT entirely since CI is happy with using an earlier OpenGL context version instead
tools/install/libdrake/BUILD.bazel, line 255 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
We also need
glew_depshere to getlibGLEW.so.2.
should no longer be relevant now that it's back to X
|
(it's probably easiest to look at this across all revisions and ignore the glut/glew stuff in the middle) |
There was a problem hiding this comment.
Looks like no additional CI changes needed? If so, we should close #12868 now.
LGTM so far.
Reviewed 12 of 14 files at r4.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform) (waiting on @tehbelinda)
geometry/render/gl_renderer/opengl_context.cc, line 64 at r3 (raw file):
Previously, tehbelinda (Bel) wrote…
should no longer be relevant now that it's back to X
Is still looks to me like context_ is never nullptr, here in the dtor?
geometry/render/gl_renderer/opengl_context.cc, line 27 at r4 (raw file):
// Helper function for loading OpenGL extension functions. template <class F> F* GetGLXFunctionARB(const char* func_name) {
nit "ARB" is quite obscure; I guess https://www.opengl.org/archives/about/arb/ is what we mean?
Anyway, https://drake.mit.edu/styleguide/cppguide.html#General_Naming_Rules says "Arb" not "ARB".
geometry/render/gl_renderer/opengl_context.cc, line 76 at r4 (raw file):
// NOTE: The consts True and False come from gl/glx.h (indirectly), but // ultimately from X11/Xlib.h. context_ = glXCreateContextAttribsARB(display(), fb_configs[0], 0, True,
Shouldn't we check fb_count before using the 0'th element?
geometry/render/gl_renderer/opengl_context.cc, line 84 at r4 (raw file):
} XFree(fb_configs);
nit Use https://github.com/RobotLocomotion/drake/blob/master/common/scope_exit.h to achieve this, moving it right after the allocation happens, to emulate RAII. That way, we don't leak if something throws (e.g. the context == nullptr just above).
geometry/render/gl_renderer/opengl_context.cc, line 90 at r4 (raw file):
MakeCurrent(); // Debug.
nit To me, we should have turned on debug logging before doing all of the operations in the constructor -- in case they have interesting diagnostics?
|
Can we close #12936? |
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri and @tehbelinda)
geometry/render/gl_renderer/opengl_context.cc, line 27 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit "ARB" is quite obscure; I guess https://www.opengl.org/archives/about/arb/ is what we mean?
Anyway, https://drake.mit.edu/styleguide/cppguide.html#General_Naming_Rules says "Arb" not "ARB".
This is maybe the best reference:
https://www.opengl.org/archives/resources/features/OGLextensions/
ARB is the roughly the last step before being included in the OpenGL specification. They are not, however, in the specification, which is why people often use GLEW when working with them.
There was a problem hiding this comment.
First pass done.
Reviewed 1 of 12 files at r2, 10 of 14 files at r4.
Reviewable status: 14 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform) (waiting on @jamiesnape, @jwnimmer-tri, and @tehbelinda)
geometry/render/gl_renderer/opengl_context.h, line 12 at r4 (raw file):
namespace internal { /** Handle OpenGL context initialization, clean-up and generic OpenGL queries.
nit: I'll die fighting for the oxford comma. :) "...initialization, clean-up, and generic..."
geometry/render/gl_renderer/opengl_context.h, line 19 at r4 (raw file):
class OpenGlContext { public: /** Constructor. Open an X display and initialize an OpenGL context. The
BTW I'm not sure how much of this documentation belongs in the constructor documentation. It's very implementation heavy. I don't think it should be talking about x windows or displays or anything of that ilk.
geometry/render/gl_renderer/opengl_context.h, line 27 at r4 (raw file):
explicit OpenGlContext(bool debug = false); /** Releases context. */
nit: I don't think any documentation is necessary on the destructor.
geometry/render/gl_renderer/opengl_context.h, line 30 at r4 (raw file):
~OpenGlContext(); /** Makes this context current or throws. */
nit: Throws under what context? What does the user need to know to feel like it won't be throwing?
geometry/render/gl_renderer/opengl_context.cc, line 64 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Is still looks to me like
context_is never nullptr, here in the dtor?
Agreed
geometry/render/gl_renderer/opengl_context.cc, line 27 at r4 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
This is maybe the best reference:
https://www.opengl.org/archives/resources/features/OGLextensions/
ARB is the roughly the last step before being included in the OpenGL specification. They are not, however, in the specification, which is why people often use GLEW when working with them.
I feel that anyone who plays with OpenGl for a living would know about the architectural review board. I'd definitely advocate a link so that other drake developers have a springboard to learn about the byzantine ways of OpenGl.
And, similarly, GLX -> Glx for the same style guide.
geometry/render/gl_renderer/opengl_context.cc, line 38 at r4 (raw file):
void GLDebugCallback(GLenum, GLenum type, GLuint, GLenum severity, GLsizei, const GLchar* message, const void*) {
nit: indentation
geometry/render/gl_renderer/opengl_context.cc, line 76 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Shouldn't we check
fb_countbefore using the 0'th element?
We could but because we're providing attributes in the call to glXChooseFBConfig we're guaranteed to have a valid result or null.
That said, either a note explaining that or a simple reality check as @jwnimmer-tri suggests would illuminate this otherwise arcane convention.
geometry/render/gl_renderer/opengl_context.cc, line 90 at r4 (raw file):
Alternatively, the documentation on the semantics of debug need to be updated:
if
debugis true, the OpenGl context will be a "debug" context, in that the OpenGl implementation's errors will be written to the Drake log. See https://www.khronos.org/opengl/wiki/Debug_Output for more information.
The point is that it has nothing to do with the acquisition of the OpenGl context, but is a property of the acquired context.
geometry/render/gl_renderer/opengl_context.cc, line 107 at r4 (raw file):
nit: This error message definitely needs more detail. In Drake the word "context" is loaded with so much meaning, that we should be clear when we use the word. :) Something like:
Error activating an OpenGl context
We might almost consider giving the context a name so that whatever things we have that acquire one provide a name so if there happen to be multiple contexts, the name can help distinguish.
geometry/render/gl_renderer/opengl_context.cc, line 133 at r4 (raw file):
// reopen the display on CI, we can't request a new OpenGL context. // This pattern won't call the corresponding `XCloseDisplay()` when the // program exiting, but it seems not so evil to skip that.
nit s/exiting/exists
geometry/render/gl_renderer/opengl_includes.h, line 4 at r4 (raw file):
Provides a one-stop shop for pulling in the OpenGl symbols in with the required extensions. Note that this distinguishes 'generic' OpenGl headers from those that are OS-based. This handles the ordering and the required
What's the note for "OS-based" mean? We only have one OS for which this is meaningful and have no artifacts in this file that reflect OS.
geometry/render/gl_renderer/test/opengl_context_test.cc, line 17 at r4 (raw file):
// Tests switching contexts and enabling debug mode. OpenGlContext debug_context{true}; debug_context.MakeCurrent();
It'd be nice to see something written to a log. At least the vendor info. But, for preference, an error message as well.
geometry/render/gl_renderer/test/opengl_context_test.cc, line 19 at r4 (raw file):
debug_context.MakeCurrent(); // Back to original context. opengl_context.MakeCurrent();
Not throwing on MakeCurrent doesn't imply successful switch. Let's throw in some OpenGl commands to change state and see if the contexts are truly independent.
The simplest test would be to enable/disable some arbitrary feature (e.g., GL_TEXTURE_2D) and testing to see if it is enabled glIsEnabled().
tools/workspace/x11/repository.bzl, line 16 at r4 (raw file):
# Only available on Ubuntu. On macOS, no targets should depend on @x11. os_result = determine_os(repo_ctx) if os_result.error != None:
BTW Preference for os_result.error is not None to avoid any potential collision with overloading __ne__()?
|
geometry/render/gl_renderer/opengl_context.cc, line 27 at r4 (raw file): Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I was thinking of keeping |
|
geometry/render/gl_renderer/opengl_context.cc, line 27 at r4 (raw file): Previously, tehbelinda (Bel) wrote…
Similarly all the |
There was a problem hiding this comment.
Reviewable status: 14 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform) (waiting on @jamiesnape, @jwnimmer-tri, and @tehbelinda)
geometry/render/gl_renderer/opengl_context.cc, line 27 at r4 (raw file):
Previously, tehbelinda (Bel) wrote…
Similarly all the
...ARBfunctions from the GL library are like that, e.g.glXGetProcAddressARB. I guess if we're going to change that, then we should also change GL->Gl for consistency in our own naming and ignore how the library functions are named.
I would still suggest we at least go withGlXsince theglandXare treated separately
I'd go for GlX, But, yeah, in our APIs GL --> Gl.
|
tools/workspace/x11/repository.bzl, line 16 at r4 (raw file): Previously, SeanCurtis-TRI (Sean Curtis) wrote…
apparently |
There was a problem hiding this comment.
Reviewable status: 14 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform) (waiting on @jamiesnape, @SeanCurtis-TRI, and @tehbelinda)
geometry/render/gl_renderer/opengl_context.cc, line 64 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Agreed
Over to @tehbelinda
geometry/render/gl_renderer/opengl_context.cc, line 27 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'd go for
GlX, But, yeah, in our APIsGL-->Gl.
Over to @tehbelinda.
For illustrating ARB I'm happy to cite any URL that makes sense, I'm not picky.
And yes per GSG, CamelNames that we invent (i.e., not GLint) should be GlContext not GLContext, GetArb not GetARB, GlDebugCallback, not GLDebugCallback, etc.
geometry/render/gl_renderer/opengl_context.cc, line 76 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
We could but because we're providing attributes in the call to
glXChooseFBConfigwe're guaranteed to have a valid result or null.That said, either a note explaining that or a simple reality check as @jwnimmer-tri suggests would illuminate this otherwise arcane convention.
Over to @tehbelinda.
Yes, the check could be DRAKE_DEMAND if we're sure it's always true -- we don't need a hand-crafted exception.
geometry/render/gl_renderer/opengl_context.cc, line 90 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Alternatively, the documentation on the semantics of
debugneed to be updated:if
debugis true, the OpenGl context will be a "debug" context, in that the OpenGl implementation's errors will be written to the Drake log. See https://www.khronos.org/opengl/wiki/Debug_Output for more information.The point is that it has nothing to do with the acquisition of the OpenGl context, but is a property of the acquired context.
Sure, if debug has no effect during the ctor here anyway, then reminding us of that in a comment sounds fine.
tools/workspace/x11/repository.bzl, line 16 at r4 (raw file):
Previously, tehbelinda (Bel) wrote…
apparently
isis not supported!ERROR: /home/belindateh/drake/tools/workspace/x11/repository.bzl:16:24: 'is' not supported, use '==' instead ERROR: /home/belindateh/drake/tools/workspace/x11/repository.bzl:16:24: syntax error at 'is': expected : ERROR: /home/belindateh/drake/tools/workspace/x11/repository.bzl:26:9: Trailing comma is allowed only in parenthesized tuples. ERROR: /home/belindateh/drake/tools/workspace/x11/repository.bzl:32:5: Trailing comma is allowed only in parenthesized tuples. ERROR: /home/belindateh/drake/tools/workspace/x11/repository.bzl:35:1: Trailing comma is allowed only in parenthesized tuples.
FYI https://docs.bazel.build/versions/master/skylark/language.html#differences-with-python
The following Python features are not supported:: is (use == instead).
tehbelinda
force-pushed
the
render-engine-gl-x11
branch
from
f25d59f
to
56ca1dd
Compare
There was a problem hiding this comment.
Yup, I've closed both those issues now
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri and @SeanCurtis-TRI)
geometry/render/gl_renderer/opengl_context.h, line 30 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Throws under what context? What does the user need to know to feel like it won't be throwing?
Not sure how detailed it should be otherwise it reveals too much of the X implementation? so I've just added this for now
https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glXMakeCurrent.xml
geometry/render/gl_renderer/opengl_context.cc, line 64 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Agreed
Ah yup, realised the destructor wouldn't get called if the constructor doesn't finish anyway
geometry/render/gl_renderer/opengl_context.cc, line 76 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
We could but because we're providing attributes in the call to
glXChooseFBConfigwe're guaranteed to have a valid result or null.That said, either a note explaining that or a simple reality check as @jwnimmer-tri suggests would illuminate this otherwise arcane convention.
Done.
geometry/render/gl_renderer/opengl_context.cc, line 90 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Alternatively, the documentation on the semantics of
debugneed to be updated:if
debugis true, the OpenGl context will be a "debug" context, in that the OpenGl implementation's errors will be written to the Drake log. See https://www.khronos.org/opengl/wiki/Debug_Output for more information.The point is that it has nothing to do with the acquisition of the OpenGl context, but is a property of the acquired context.
Yeah that's much clearer, thanks
geometry/render/gl_renderer/opengl_context.cc, line 107 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This error message definitely needs more detail. In Drake the word "context" is loaded with so much meaning, that we should be clear when we use the word. :) Something like:
Error activating an OpenGl context
We might almost consider giving the context a name so that whatever things we have that acquire one provide a name so if there happen to be multiple contexts, the name can help distinguish.
I think the only unique id is the pointer address itself? could just print that out
geometry/render/gl_renderer/opengl_includes.h, line 4 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
What's the note for "OS-based" mean? We only have one OS for which this is meaningful and have no artifacts in this file that reflect OS.
Rewording since it's not so relevant here, I'm guessing it was alluding to something like in vtk where it has different implementations that are pulled in depending on the platform
geometry/render/gl_renderer/test/opengl_context_test.cc, line 17 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
It'd be nice to see something written to a log. At least the vendor info. But, for preference, an error message as well.
It sounds like there's no good way to do this
https://drakedevelopers.slack.com/archives/C2CK4CWE7/p1584464181012800
So I propose checking that the GL_DEBUG_OUTPUT feature is enabled?
geometry/render/gl_renderer/test/opengl_context_test.cc, line 19 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Not throwing on
MakeCurrentdoesn't imply successful switch. Let's throw in some OpenGl commands to change state and see if the contexts are truly independent.The simplest test would be to enable/disable some arbitrary feature (e.g., GL_TEXTURE_2D) and testing to see if it is enabled
glIsEnabled().
Done.
tools/workspace/x11/repository.bzl, line 16 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
FYI https://docs.bazel.build/versions/master/skylark/language.html#differences-with-python
The following Python features are not supported::
is(use == instead).
thanks! 👍
There was a problem hiding this comment.
Reviewed 1 of 14 files at r3, 5 of 5 files at r5.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri and @tehbelinda)
geometry/render/gl_renderer/opengl_context.h, line 30 at r4 (raw file):
Previously, tehbelinda (Bel) wrote…
Not sure how detailed it should be otherwise it reveals too much of the X implementation? so I've just added this for now
https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glXMakeCurrent.xml
At the very least, we should communicate if it's due to the user misusing the API or if it's beyond the scope of the program.
https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glXMakeCurrent.xml
This suggests that misuse of the API is possible under several conditions:
- The context is made current in multiple contexts simultaneously.
- If we attempt to call this inside a
glBegin()glEnd()block. - If the state is
GLX_FEEDBACKorGLX_SELECT(I'm not sure about this; I knowGL_SELECTandGL_FEEDBACKbut am unsure of what theGLXvariant is -- just aliases perhaps? - The current context has GL commands that cannot be flushed because the old context's drawable has been destroyed. This seems highly unlikely as our display has static storage duration.
Things beyond control:
- We run out of resources.
There are a surprising number of ways for a user to mess this up. Even if not overly likely.
Thoughts?
geometry/render/gl_renderer/opengl_context.cc, line 107 at r4 (raw file):
Previously, tehbelinda (Bel) wrote…
I think the only unique id is the pointer address itself? could just print that out
Nah. We have no helpful unique identifiers. :) Just communicating it's an OpenGl context is good enough for now.
geometry/render/gl_renderer/opengl_context.cc, line 65 at r5 (raw file):
GLXFBConfig* fb_configs = glXChooseFBConfig( display(), DefaultScreen(display()), kVisualAttribs, &fb_count); ScopeExit guard([fb_configs]() { XFree(fb_configs); });
BTW This gives me a warm, fuzzy feeling. :)
geometry/render/gl_renderer/opengl_context.cc, line 82 at r5 (raw file):
// are guaranteed to have a valid result in fb_configs as we have already // checked for null. DRAKE_DEMAND(fb_count > 0);
BTW In fact, it should be 1. While this test satisfies that condition, the implication is we believe we might get more than one...perhaps avoiding that implication?
geometry/render/gl_renderer/opengl_includes.h, line 4 at r4 (raw file):
I'm still balking at the language. It doesn't "distinguish" because there's only a single set of data. There's no "either/or". It's just one blob of includes. How about:
By design, this includes only the baseline OpenGl headers. If code requires more elaborate libraries, it is responsible for pulling them in directly.
That said, I'm going to mark myself satisfied and leave it up to you. All of this is dancing around "We're not including glx.h here because we want to keep it isolated."
geometry/render/gl_renderer/opengl_includes.h, line 2 at r5 (raw file):
/** @file Provides a one-stop shop for pulling in the OpenGl symbols in with the
nit: (Just noticed) "...pulling in the OpenGl symbols in with the..."
geometry/render/gl_renderer/test/opengl_context_test.cc, line 17 at r4 (raw file):
Previously, tehbelinda (Bel) wrote…
It sounds like there's no good way to do this
https://drakedevelopers.slack.com/archives/C2CK4CWE7/p1584464181012800
So I propose checking that the GL_DEBUG_OUTPUT feature is enabled?
Suits me.
tools/workspace/x11/repository.bzl, line 16 at r4 (raw file):
Previously, tehbelinda (Bel) wrote…
thanks! 👍
TIL
Skylark keeps seducing me with its python-like ways.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r5.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @tehbelinda)
|
geometry/render/gl_renderer/opengl_includes.h, line 4 at r4 (raw file): Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I think the other aspect of this is the ordering - e.g. GL/gl.h has to be first |
|
geometry/render/gl_renderer/opengl_context.h, line 30 at r4 (raw file): Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Would it be simpler to just link to https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glXMakeCurrent.xml? |
tehbelinda
force-pushed
the
render-engine-gl-x11
branch
from
56ca1dd
to
2d4b87f
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r6.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)
|
|
|
I guess there can be more than one! |
tehbelinda
force-pushed
the
render-engine-gl-x11
branch
from
2d4b87f
to
af3029d
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)
|
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please |
There was a problem hiding this comment.
The "more than one" raises more questions than it answers. How do they differ? is it really to our advantage to pick the first one?
That said, go ahead and pick just the first one, put a note indicating that there may be multiple and, maybe, someday, someone should look at the others. (The first one seems to have served us reasonably well up to this point).
Otherwise:
Reviewable status:
complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)
geometry/render/gl_renderer/opengl_context.h, line 30 at r4 (raw file):
Previously, tehbelinda (Bel) wrote…
Would it be simpler to just link to https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glXMakeCurrent.xml?
Go for it; it's an internal:: class, so anyone spelunking in that corner of the code gets to know implementation details and should be able to understand that page.
geometry/render/gl_renderer/opengl_includes.h, line 4 at r4 (raw file):
Previously, tehbelinda (Bel) wrote…
I think the other aspect of this is the ordering - e.g. GL/gl.h has to be first
Right -- I wasn't meaning to ignore that, I was just addressing the persistence of OS appearing here. The order is what makes the baseline OpenGl work.
This has been split up from render-gl-port to keep PR size down
This change is