Adding option for EGL and OpenGL preference. by prabhuramachandran · Pull Request #22 · KitwareMedical/VTKPythonPackage

prabhuramachandran · 2018-10-09T19:39:57Z

No description provided.

jcfr · 2018-10-09T19:41:22Z

Thanks for the PR 👍

jcfr · 2018-10-09T19:41:32Z

Thanks for the PR 👍

jcfr · 2018-10-09T19:41:54Z


+  if(OpenGL_GL_PREFERENCE)
+    list(APPEND ep_common_cmake_cache_args
+      -DOpenGL_GL_PREFERENCE:STRING=${OpenGL_GL_PREFERENCE})


The variable OpenGL_GL_PREFERENCE doesn't seem to be set anywhere ? Should an an explicit option be added ?

It is being added to ep_common_cmake_cache_args which is used for the VTK build.

https://github.com/KitwareMedical/VTKPythonPackage/blob/master/CMakeLists.txt#L226

The option is used if you want to use GLVND vs the classic OpenGL. So one could do:
VTK_CMAKE_ARGS="-DOpenGL_GL_PREFERENCE:STRING=GLVND" python setup.py ...

Would you prefer a separate config variable that is explicitly set?

Setting VTK_CMAKE_ARGS env. variable make sense, you could also pass the options to setup.py using python setup.py bdist_wheel -- -DOpenGL_GL_PREFERENCE:STRING=GLVND (see doc)

But, it would also be great to add the variable OpenGL_GL_PREFERENCE here:

VTKPythonPackage/CMakeLists.txt

Lines 46 to 70 in 9a93fbe

#-----------------------------------------------------------------------------

# Options

# When building different "flavor" of VTK python packages on a given platform,

# explicitly setting the following options allow to speed up package generation by

# re-using existing resources.

#

# VTK_SOURCE_DIR: Path to an existing source directory

# VTK_Group_Web: Whether web package should be included

# VTK_OPENGL_HAS_OSMESA: Whether to use OSMesa for offscreen rendering

#

option(VTKPythonPackage_BUILD_PYTHON "Build VTK python module" ON)

mark_as_advanced(VTKPythonPackage_BUILD_PYTHON)

option(VTK_Group_Web "Build VTK web module" OFF)

mark_as_advanced(VTK_Group_Web)

option(VTK_OPENGL_HAS_OSMESA "Use OSMesa for offscreen rendering" OFF)

mark_as_advanced(VTK_OPENGL_HAS_OSMESA)

if(VTK_OPENGL_HAS_OSMESA OR APPLE OR WIN32)

set(VTK_USE_X OFF)

else()

set(VTK_USE_X ON)

endif()

Consider also listing all known options for OpenGL_GL_PREFERENCE and add any relevant documentation links.

It also seems this option is linux specific: https://cmake.org/cmake/help/latest/module/FindOpenGL.html?highlight=opengl_gl_preference#linux-specific

Alright, I will add it as a separate option and mention the valid options. But will append it to the ep_common_cmake_cache_args on Linux alone as it cannot be added as an explicit VTK option the way it is currently passed.

prabhuramachandran · 2018-10-09T21:16:47Z

Made changes, please let me know if this is good.

jcfr

Thanks. Almost there 🏎️

jcfr · 2018-10-09T22:40:58Z

+  # This is a Linux only option and the valid values can be either
+  # "LEGACY" or "GLVND". More information here:
+  # https://cmake.org/cmake/help/git-stage/policy/CMP0072.html
+  option(OpenGL_GL_PREFERENCE "Set the preferred OpenGL library" "LEGACY")


Almost. Option are only boolean.

Correct syntax would be:

set(OpenGL_GL_PREFERENCE "LEGACY" CACHE "Set the preferred OpenGL library")

Also, since this is linux only ... consider the following:

if(UNIX AND NOT APPLE) set(OpenGL_GL_PREFERENCE "LEGACY" CACHE STRING "Set the preferred OpenGL library") endif()

And finally, you could even add an extra check for invalid values and give some hints for cmake-gui, ccache ...

if(UNIX AND NOT APPLE) set(OpenGL_GL_PREFERENCE "LEGACY" CACHE STRING "Set the preferred OpenGL library") if(NOT OpenGL_GL_PREFERENCE MATCHES "^(LEGACY|GLVND)$") message(FATAL_ERROR "Unknown value for OpenGL_GL_PREFERENCE. Valid values are LEGACY or GLVND") endif() # Set the possible values of build type for cmake-gui set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "LEGACY" "GLVND") endif()

Ahh, thank you, my ignorance of CMake is showing. I will make these fixes and update the PR.

jcfr · 2018-10-09T22:42:28Z

    set(VTK_USE_X ON)
  endif()

+  if(UNIX AND OpenGL_GL_PREFERENCE)


if(UNIX AND NOT APPLE) list(APPEND ep_common_cmake_cache_args -DOpenGL_GL_PREFERENCE:STRING=${OpenGL_GL_PREFERENCE}) endif()

Thanks, you've done all the work. Sorry for a sloppy PR.

jcfr · 2018-10-10T14:41:05Z

Thanks for your patience 👍

jcfr · 2018-10-10T14:43:34Z

+
+  # This is a Linux only option and the valid values can be either
+  # "LEGACY" or "GLVND". More information here:
+  # https://cmake.org/cmake/help/git-stage/policy/CMP0072.html


One last thing, since was introduced in CMake 3.11 ... could you also bump the minimum CMake version from 3.3 to 3.11 ?

See https://cmake.org/cmake/help/v3.13/manual/cmake-policies.7.html#policies-introduced-by-cmake-3-11

Well, doesn't it only say that with 3.11 the default changes from legacy to glvnd? It doesn't say that the option is not valid before 3.11.

Good point. That would be 3.10 then, see https://cmake.org/cmake/help/v3.10/release/3.10.html#modules

Done! Thanks.

This is the version that supports the OpenGL_GL_PREFERENCE option.

Adding option for EGL and OpenGL preference.

db3883b

prabhuramachandran requested a review from jcfr October 9, 2018 19:40

jcfr reviewed Oct 9, 2018

View reviewed changes

Modify as per review.

85140da

jcfr requested changes Oct 9, 2018

View reviewed changes

prabhuramachandran and others added 2 commits October 10, 2018 09:33

Define the OpenGL_GL_PREFERENCE correctly.

5fd8266

Merge branch 'master' into add-egl-option

1759ed8

jcfr approved these changes Oct 10, 2018

View reviewed changes

jcfr reviewed Oct 10, 2018

View reviewed changes

Bump required version of CMake to 3.10.

567100f

This is the version that supports the OpenGL_GL_PREFERENCE option.

jcfr merged commit b16b084 into KitwareMedical:master Oct 10, 2018

prabhuramachandran deleted the add-egl-option branch October 10, 2018 21:09

	#-----------------------------------------------------------------------------
	# Options

	# When building different "flavor" of VTK python packages on a given platform,
	# explicitly setting the following options allow to speed up package generation by
	# re-using existing resources.
	#
	# VTK_SOURCE_DIR: Path to an existing source directory
	# VTK_Group_Web: Whether web package should be included
	# VTK_OPENGL_HAS_OSMESA: Whether to use OSMesa for offscreen rendering
	#

	option(VTKPythonPackage_BUILD_PYTHON "Build VTK python module" ON)
	mark_as_advanced(VTKPythonPackage_BUILD_PYTHON)

	option(VTK_Group_Web "Build VTK web module" OFF)
	mark_as_advanced(VTK_Group_Web)

	option(VTK_OPENGL_HAS_OSMESA "Use OSMesa for offscreen rendering" OFF)
	mark_as_advanced(VTK_OPENGL_HAS_OSMESA)
	if(VTK_OPENGL_HAS_OSMESA OR APPLE OR WIN32)
	set(VTK_USE_X OFF)
	else()
	set(VTK_USE_X ON)
	endif()

Conversation

prabhuramachandran commented Oct 9, 2018

Uh oh!

jcfr commented Oct 9, 2018

Uh oh!

jcfr commented Oct 9, 2018

Uh oh!

jcfr Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prabhuramachandran commented Oct 9, 2018

Uh oh!

jcfr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcfr commented Oct 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jcfr Oct 9, 2018 •

edited

Loading