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

OpenGL renderer a.k.a. shader support #1382

Merged
merged 21 commits into from Apr 17, 2021
Merged

OpenGL renderer a.k.a. shader support #1382

merged 21 commits into from Apr 17, 2021

Conversation

Copy link
Contributor

@ts-korhonen ts-korhonen commented Apr 16, 2021

Summary

I've been working on shader support via renderer that creates OpenGL context window allowing use of OpenGL library. It is now in a state where it seems to be working while there's still more to be done. Currently it replaces the existing SDL OpenGL option in menus and has no additional UI. I have selected the libretro shader files as a target for compatibility.

Implemented features in current state:

  • Rendering in OpenGL (3.3 core)
  • Basic shader support for libretro format shader files
    • Single-pass, no previous frames or scaling
    • path is hardcoded (shaders/shader.glsl)

Planned features:

  • Menu options & saving them in config
  • Reading .glslp files for settings
  • Multiple shader passes
  • Keeping previous frames

I didn't want to start modifying the UI yet and I opened this draft PR hoping to get some feedback from core developers.
Should this current state be merged into main repo, maybe in the experimental build?
Should it be it's own option and not replace the current SDL OpenGL option like it's doing now?

Checklist

References

Provide links to datasheets or other documentation that helped you implement this pull request.

@OBattler
Copy link
Collaborator

@OBattler OBattler commented Apr 16, 2021

I do believe it should very much be merged into the repo, perhaps behind #ifdef DEV_BRANCH so only the Dev build has the feature, and I do agree with the idea of having it as a separate option.

@dhrdlicka
Copy link
Contributor

@dhrdlicka dhrdlicka commented Apr 16, 2021

I am concerned that having both SDL/OpenGL and pure OpenGL as available options might generate unnecessary confusion, especially if your OpenGL renderer supports shaders, which the existing SDL option does not. Is there any real benefit in keeping both?

@OBattler
Copy link
Collaborator

@OBattler OBattler commented Apr 16, 2021

The current OpenGL (which is 2.0, I believe) in some case performs better than 3.0, so I do think there should be a choice.

Copy link
Contributor

@dhrdlicka dhrdlicka left a comment

I noticed that the MSVC build is broken -- if you could fix it then it would be awesome, though it's not really that important.

src/win/win_opengl.c Outdated Show resolved Hide resolved
src/win/win_opengl.c Outdated Show resolved Hide resolved
src/win/win_opengl.c Outdated Show resolved Hide resolved
src/win/win_opengl.c Outdated Show resolved Hide resolved
src/win/win_opengl_glslp.c Show resolved Hide resolved
@dhrdlicka
Copy link
Contributor

@dhrdlicka dhrdlicka commented Apr 16, 2021

The current OpenGL (which is 2.0, I believe) in some case performs better than 3.0, so I do think there should be a choice.

In that case it might be better to adapt the new renderer from this PR so there is feature parity.

@ts-korhonen would it be possible/easy to add OpenGL 2.0 support to your current code?

@ts-korhonen
Copy link
Contributor Author

@ts-korhonen ts-korhonen commented Apr 16, 2021

@ts-korhonen would it be possible/easy to add OpenGL 2.0 support to your current code?

I don't think so, it's OpenGL Core that is newer. I think keeping the old one for that is better.

@OBattler
Copy link
Collaborator

@OBattler OBattler commented Apr 16, 2021

I don't think so, it's OpenGL Core that is newer. I think keeping the old one for that is better.

I agree. The old one should be kept in addition to the new one.

@ts-korhonen ts-korhonen marked this pull request as ready for review Apr 17, 2021
@OBattler OBattler merged commit 027d931 into 86Box:master Apr 17, 2021
26 checks 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.

None yet

3 participants