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

consolidate EGL init/state #138

Merged
merged 4 commits into from
Feb 1, 2025
Merged

Conversation

ikalco
Copy link
Contributor

@ikalco ikalco commented Jan 24, 2025

next step is to use CEGLRenderer in HL

reasoning: egl init, loading, and egl specific funcs (createEGLImage) are all in one place

note:
egl queried formats don't have external modifiers stripped here, but hl impl does, so will deal with it in hl pr

@vaxerski
Copy link
Member

I don't see the point? Breaks ABI, API, and what's the benefit?

@ikalco
Copy link
Contributor Author

ikalco commented Jan 30, 2025

completely forgot about this lol
my original thought was that egl init would all be in one place and it could possibly allow for using multiple renderers in hl later or maybe lead to implementing different renderers under a parent class

but i don't want to do all that right now, so abi can be broken then if me or someone else ever tries

after removing the abi breaking stuff, I would say this is just an overall improvement
this should also fix hyprwm/Hyprland#9239 (it crashes in Renderer dtor, prob from eglTerminate on null)

@ikalco ikalco force-pushed the move_to_eglrenderer branch from 39d5694 to f7433f2 Compare January 31, 2025 04:37
@ikalco ikalco force-pushed the move_to_eglrenderer branch from f7433f2 to e93a32f Compare January 31, 2025 04:39
@ikalco ikalco changed the title consolidate EGL init/state into CEGLRendrerer consolidate EGL init/state <s>into CEGLRendrerer</s> Jan 31, 2025
@ikalco ikalco changed the title consolidate EGL init/state <s>into CEGLRendrerer</s> consolidate EGL init/state Jan 31, 2025
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically you break CRenderer abi but is that even used by hl directly?

@ikalco
Copy link
Contributor Author

ikalco commented Feb 1, 2025

uuh CDRMRenderer from aq? no HL doesn't use that directly
my original thought was to move the egl init in CHyprOpenGLImpl to aq by exposing CDRMRenderer (but also renamed it to CEGLRenderer to be more accurate)

@vaxerski
Copy link
Member

vaxerski commented Feb 1, 2025

no, dont

@vaxerski vaxerski merged commit 95506e5 into hyprwm:main Feb 1, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants