[gpu] Refactor and fix OpenGL backend using Skia - #193
Conversation
aseprite-bot
left a comment
There was a problem hiding this comment.
clang-tidy made some suggestions
|
|
||
| inline bool handle_toggle_gpu_key(const os::Event& ev) | ||
| { | ||
| os::WindowRef w = ev.window(); |
There was a problem hiding this comment.
warning: the variable 'w' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [performance-unnecessary-copy-initialization]
| os::WindowRef w = ev.window(); | |
| const os::WindowRef& w = ev.window(); |
|
|
||
| inline bool handle_toggle_gpu_key(const os::Event& ev) | ||
| { | ||
| os::WindowRef w = ev.window(); |
There was a problem hiding this comment.
warning: variable 'w' of type 'os::WindowRef' (aka 'Refos::Window') can be declared 'const' [misc-const-correctness]
| os::WindowRef w = ev.window(); | |
| os::WindowRef const w = ev.window(); |
| GLContextGLX(::Display* display) : m_display(display) {} | ||
|
|
||
| ~GLContextGLX() { destroyGLContext(); } | ||
| ~GLContextGLX() { destroyContext(); } |
There was a problem hiding this comment.
warning: Call to virtual method 'GLContextGLX::destroyContext' during destruction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]
~GLContextGLX() { destroyContext(); }
^Additional context
os/gl/gl_context_glx.h:23: Call to virtual method 'GLContextGLX::destroyContext' during destruction bypasses virtual dispatch
~GLContextGLX() { destroyContext(); }
^| bool createGLContext() override | ||
| bool makeContext(Window* window, GpuContext* shared) override | ||
| { | ||
| auto sharedGpuContext = (shared ? (GLXContext)shared->nativeHandle() : nullptr); |
There was a problem hiding this comment.
warning: 'auto sharedGpuContext' can be declared as 'auto *sharedGpuContext' [readability-qualified-auto]
| auto sharedGpuContext = (shared ? (GLXContext)shared->nativeHandle() : nullptr); | |
| auto *sharedGpuContext = (shared ? (GLXContext)shared->nativeHandle() : nullptr); |
| GLContextNSGL(); | ||
| ~GLContextNSGL(); | ||
|
|
||
| void setView(id view); |
There was a problem hiding this comment.
warning: unknown type name 'id' [clang-diagnostic-error]
void setView(id view);
^| virtual ~GpuContext() {} | ||
| virtual bool isValid() { return false; } | ||
|
|
||
| virtual bool makeContext(Window* window, GpuContext* shared) { return false; } |
There was a problem hiding this comment.
warning: parameter 'window' is unused [misc-unused-parameters]
| virtual bool makeContext(Window* window, GpuContext* shared) { return false; } | |
| virtual bool makeContext(Window* /*window*/, GpuContext* shared) { return false; } |
| virtual void makeCurrent(Window* window) {} | ||
| virtual void swapBuffers(Window* window) {} | ||
| virtual void flush() {} | ||
| virtual RenderTarget makeRenderTarget(const gfx::Size& size, |
There was a problem hiding this comment.
warning: parameter 'size' is unused [misc-unused-parameters]
| virtual RenderTarget makeRenderTarget(const gfx::Size& size, | |
| virtual RenderTarget makeRenderTarget(const gfx::Size& /*size*/, |
| virtual void swapBuffers(Window* window) {} | ||
| virtual void flush() {} | ||
| virtual RenderTarget makeRenderTarget(const gfx::Size& size, | ||
| const int scale, |
There was a problem hiding this comment.
warning: parameter 'scale' is unused [misc-unused-parameters]
| const int scale, | |
| const int /*scale*/, |
| virtual void flush() {} | ||
| virtual RenderTarget makeRenderTarget(const gfx::Size& size, | ||
| const int scale, | ||
| const os::ColorSpaceRef& cs) |
There was a problem hiding this comment.
warning: parameter 'cs' is unused [misc-unused-parameters]
| const os::ColorSpaceRef& cs) | |
| const os::ColorSpaceRef& /*cs*/) |
| m_screenMousePos = ev.window()->pointToScreen(ev.position()); | ||
| } | ||
|
|
||
| std::unique_ptr<GpuContext> SystemWin::makePlatformGpuContext() |
There was a problem hiding this comment.
warning: method 'makePlatformGpuContext' can be made static [readability-convert-member-functions-to-static]
| std::unique_ptr<GpuContext> SystemWin::makePlatformGpuContext() | |
| static std::unique_ptr<GpuContext> SystemWin::makePlatformGpuContext() |
* New os::GpuContext class to wrap native OpenGL context * New os::SkiaGpuContext class to wrap Skia GrDirectContext * Added os::System::makePlatformGpuContext() * Added os::Window::clientSize() * Fixed makeCurrent/swapBuffers logic * Fixed support for multiple windows * macOS support doesn't work Related to aseprite/aseprite#960
It partially works only on Linux at the moment.
aseprite-bot
left a comment
There was a problem hiding this comment.
clang-tidy made some suggestions
| p.color(gfx::rgba(0, 0, 0)); | ||
| surface->drawRect(gfx::Rect(rc.x, rc.y + rc.h - h, rc.w, h), p); | ||
| if (m_textLog.size() >= m_maxlines) { | ||
| int h = m_lineHeight * newlines; |
There was a problem hiding this comment.
warning: variable 'h' of type 'int' can be declared 'const' [misc-const-correctness]
| int h = m_lineHeight * newlines; | |
| int const h = m_lineHeight * newlines; |
| #define WGL_CONTEXT_CORE_PROFILE_BIT_ARB 0x00000001 | ||
| #define WGL_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB 0x00000002 | ||
|
|
||
| using PFNWGLCREATECONTEXTATTRIBSARBPROC = HGLRC(WINAPI*)(HDC, HGLRC, const int*); |
There was a problem hiding this comment.
warning: parameter 'window' is unused [misc-unused-parameters]
| using PFNWGLCREATECONTEXTATTRIBSARBPROC = HGLRC(WINAPI*)(HDC, HGLRC, const int*); | |
| bool makeContext(Window* /*window*/, GpuContext* shared) override |
| @@ -81,22 +103,27 @@ class GLContextWGL : public GLContext { | |||
| } | |||
There was a problem hiding this comment.
warning: parameter 'window' is unused [misc-unused-parameters]
| } | |
| void swapBuffers(Window* /*window*/) override |
…rget() Now it's possible to create offscreen framebuffers by clients.
aseprite-bot
left a comment
There was a problem hiding this comment.
clang-tidy made some suggestions
| virtual void swapBuffers(Window* window) {} | ||
| virtual void flush() {} | ||
|
|
||
| virtual SurfaceRef makeOnscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& cs) |
There was a problem hiding this comment.
warning: parameter 'cs' is unused [misc-unused-parameters]
| virtual SurfaceRef makeOnscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& cs) | |
| virtual SurfaceRef makeOnscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& /*cs*/) |
| virtual void swapBuffers(Window* window) {} | ||
| virtual void flush() {} | ||
|
|
||
| virtual SurfaceRef makeOnscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& cs) |
There was a problem hiding this comment.
warning: parameter 'size' is unused [misc-unused-parameters]
| virtual SurfaceRef makeOnscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& cs) | |
| virtual SurfaceRef makeOnscreenRenderTarget(const gfx::Size& /*size*/, const os::ColorSpaceRef& cs) |
| return {}; | ||
| } | ||
|
|
||
| virtual SurfaceRef makeOffscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& cs) |
There was a problem hiding this comment.
warning: parameter 'cs' is unused [misc-unused-parameters]
| virtual SurfaceRef makeOffscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& cs) | |
| virtual SurfaceRef makeOffscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& /*cs*/) |
| return {}; | ||
| } | ||
|
|
||
| virtual SurfaceRef makeOffscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& cs) |
There was a problem hiding this comment.
warning: parameter 'size' is unused [misc-unused-parameters]
| virtual SurfaceRef makeOffscreenRenderTarget(const gfx::Size& size, const os::ColorSpaceRef& cs) | |
| virtual SurfaceRef makeOffscreenRenderTarget(const gfx::Size& /*size*/, const os::ColorSpaceRef& cs) |
* Fixed using shared OpenGL contexts with [NSOpenGLContext initWithFormat...] * NSView requires an wantsUpdateLayer/updateLayer when using GPU acceleration to call [NSOpenGLContext update]. There is still a bug/crash when we go back to CPU/raster backend after enabling the GPU acceleration.
Related to aseprite/aseprite#960