Skip to content
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

Splitting out GL and Trade functionality to separate libraries #233

Merged
merged 51 commits into from May 1, 2018
Merged

Conversation

mosra
Copy link
Owner

@mosra mosra commented Mar 1, 2018

This is the first huge potentially breaking thing on the path to Vulkan support. Basically everything from the Magnum namespace that had anything to do with OpenGL is now moved into a new Magnum::GL library. That goes similarly for include paths and CMake imported targets. But don't worry too much (just a little), this is done with the utmost care to preserve as much backwards compatibility as possible.

I'm doing this one little step at a time to avoid getting crazy. Things to do, in order:

  • Separate Trade library out of Magnum (mostly just buildsystem changes, resulting in a new Magnum::Trade CMake imported target (and library)
  • Move everything GL-related to Magnum/GL subdir and a new library (at this step this means even the Image class and everything, which means also the whole Trade library and all plugins now depend on GL library), not adding a new namespace yet
  • Add a new Magnum::GL namespace
    • Convert Magnum::Extensions::GL::* to Magnum::GL::Extensions::* ?
    • Convert Platform::Context to Platform::GLContext
  • Provide deprecated compatibility headers (e.g. <Magnum/Buffer.h> that points to <Magnum/GL/Buffer.h>)
  • Provide deprecated aliases (e.g. Magnum::Buffer that is a typedef to Magnum::GL::Buffer)
  • Adapt rest of the project to not use deprecated headers and aliases (again, until this is done, the non-deprecated build will fail)
  • Adapt Developers Guide to use new file locations and namespaces
  • Adapt all docs to not use deprecated headers and aliases (Doxygen won't complain because of the compat versions)
    • Adapt all @section identifiers to use the GL namespace
    • Rename @extension to @gl_extension
  • Extract PixelFormat / PixelType enums back to root namespace and provide some mapping to GL types (need to check what Vulkan has here)
  • Separate GL-specific stuff out of PixelStorage and put the rest back to root namespace
    • Temporary assertions for pixel storage things that are not supported in ES / WebGL (will be later replaced with proper implementation coming from the Vulkan part)
  • Put Image classes back to root namespace
    • also tests
  • Separate MeshPrimitive and Mesh::IndexType enum out of GL lib
  • Think about how to separate sampler state out of GL lib (also enums straight in Trade, how to map to GL?) It's a bunch of Sampler enums in the root namespace now
  • Now that Trade does not depend on GL anymore, fix the linking and fix that also Primitives and all plugins
  • Verify that all dependent repos and plugins and examples build with the new lib w/o any source change
  • Update all packages and CIs to explicitly use WITH_GL and WITH_TRADE no need to, this is enabled by default
  • Introduce TARGET_GL option, allowing to enable/disable GL support in other libraries
  • Make the SDL2 and GLFW app usable with TARGET_GL being turned off (i.e., so it doesn't even link to GL)
  • Look in the OpenGL / platform overview docs and check that nothing is terribly wrong
    • Avoid saying that GL is the "core part", it's not
    • Adapt magnum-gl-info "screenshot"
  • Rename magnum-info to magnum-gl-info. Backwards compatibility for this is not considered, please complain if you need that.
  • Rename all MAGNUM_ASSERT_*_SUPPORTED() macros to MAGNUM_ASSERT_GL_*_SUPPORTED() or MAGNUM_GL_*() (how to handle backwards compat?)
    • also MAGNUM_VERIFY_NO_ERROR()
  • Changelog entries for all of above

After all the above this PR could be merged back to master, I think. Separating the remaining libraries from GL will be done incrementally over time.

Things I'm not sure how to do:

  • Backward compatibility so people linking to Magnum will be linked to Magnum::GL and Magnum::Trade as well -- there would be a cyclic dependency with this, so nope
  • Should GL::BufferImage have GL::PixelFormat or Magnum::PixelFormat? The former would require a mapping table back to Magnum::PixelFormat, the latter will feel alien (formatExtra etc.) It's the GL-specific format now.

Cc: @Squareys

@mosra mosra added this to the 2018.0b milestone Mar 1, 2018
@mosra mosra added this to TODO in GL via automation Mar 1, 2018
@mosra mosra added this to TODO in Vulkan via automation Mar 1, 2018
@mosra mosra force-pushed the split branch 3 times, most recently from 50d83af to 421031c Compare March 1, 2018 15:44
@codecov-io
Copy link

codecov-io commented Mar 1, 2018

Codecov Report

Merging #233 into master will increase coverage by 0.78%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   45.84%   46.62%   +0.78%     
==========================================
  Files         345      355      +10     
  Lines       15261    15539     +278     
==========================================
+ Hits         6996     7245     +249     
- Misses       8265     8294      +29
Impacted Files Coverage Δ
.../DebugTools/Implementation/AbstractBoxRenderer.cpp 0% <ø> (ø) ⬆️
src/Magnum/GL/Attribute.h 69.23% <ø> (ø)
src/Magnum/Audio/Renderer.h 100% <ø> (ø) ⬆️
src/Magnum/Audio/Context.h 100% <ø> (ø) ⬆️
src/Magnum/GL/Implementation/State.h 0% <ø> (ø)
src/Magnum/GL/BufferImage.h 5.47% <ø> (ø)
src/Magnum/GL/AbstractQuery.cpp 5.76% <ø> (ø)
src/Magnum/GL/BufferImage.cpp 6.34% <ø> (ø)
src/Magnum/DebugTools/ForceRenderer.h 0% <ø> (ø) ⬆️
...agnum/GL/Implementation/TransformFeedbackState.cpp 0% <ø> (ø)
... and 215 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58ff5a2...8018b17. Read the comment docs.

@mosra mosra mentioned this pull request Mar 1, 2018
27 tasks
@mosra mosra moved this from TODO to In Progress in Vulkan Mar 1, 2018
@mosra mosra self-assigned this Mar 1, 2018
@mosra mosra moved this from In Progress to Important TODO in Vulkan Mar 1, 2018
@mosra mosra moved this from TODO to In Progress in GL Mar 1, 2018
@mosra mosra force-pushed the split branch 7 times, most recently from 5480bbd to 7ab7f22 Compare March 2, 2018 21:20
@mosra mosra mentioned this pull request Mar 2, 2018
71 tasks
@mosra mosra force-pushed the split branch 2 times, most recently from c792bc9 to e425f37 Compare March 6, 2018 23:12
@mosra mosra force-pushed the split branch 2 times, most recently from 3917fe9 to 3c3105b Compare March 28, 2018 19:55
@mosra
Copy link
Owner Author

mosra commented Apr 15, 2018

Trade library split was merged to master in 920db7a.

mosra added 21 commits May 1, 2018 01:37
Primitives, MeshTools, Trade and TextureTools are not depending on the
GL library anymore.
For 2018.02 it's still referencing them, but the older changelogs are
simply converted to inline code for all references.
Because now with the generic formats all images that are in
PixelFormat::R8Unorm are translated to GL::PixelFormat::Luminance on ES2
and WebGL 1. The DistanceFieldGlyphCache still has the original, but
that one didn't really work there in the first place. That'll get
patched later.
Previously, when requesting OpenGLTester, the script correctly found
that it needs some Windowless*Application and a GL library and added
them as dependencies to OpenGLTester. But it didn't handle the
dependency of Windowless*Application on GL, which caused GL to be linked
*before* Windowless*Application, causing linker to complain that
Windowless*Application needs some more stuff from GL.
…ion.

The Platform::*Application::Configuration class was split into
Configuration and GLConfiguration, the latter containing only
GL-specific configuration. Moreover, createContext() and
tryCreateContext() were renamed to create() / tryCreate().

There's now a constructor and a create() / tryCreate() overload taking
GLConfiguration and this will be later extended with VkConfiguration,
for example. GL-specific getters/setters from Configuration are now
marked as deprecated and merged into GLConfiguration during context
creation.

Everything has still hard dependency on GL, that will be done in the
next commits.
So it's clear that this is not a Vulkan extension link.
@mosra mosra merged commit 8018b17 into master May 1, 2018
GL automation moved this from In Progress to Done May 1, 2018
Vulkan automation moved this from Important TODO to Done May 1, 2018
@mosra
Copy link
Owner Author

mosra commented May 1, 2018

Expecting angry people with pitchforks and torches doing a witch hunt on me now, haha 🙈 🔥

Documentation and proper release notes / upgrading guide coming soon with the 2018.04 release.

@mosra mosra changed the title [WIP] Splitting out GL and Trade functionality to separate libraries Splitting out GL and Trade functionality to separate libraries May 1, 2018
@mosra mosra deleted the split branch May 4, 2018 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GL
  
Done
Vulkan
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants