Skip to content

Fixes OpenGL #2023

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

Merged
merged 4 commits into from
Nov 3, 2023
Merged

Fixes OpenGL #2023

merged 4 commits into from
Nov 3, 2023

Conversation

MaximilianEmel
Copy link
Contributor

@MaximilianEmel MaximilianEmel commented Nov 2, 2023

PR Details

Updates Stride.Graphics' OpenGL to match the new version of the API that's used in Silk.NET-2.17.1

Description

  • SearchPathContainer is now an array
  • Replaced specific enums with the generic GLEnum

Related Issue

#1911

Motivation and Context

When updating OpenXR to the latest version, Silk's OpenGL bindings also got updated, which updated the version of OpenGL, which had breaking changes.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@MaximilianEmel
Copy link
Contributor Author

MaximilianEmel commented Nov 2, 2023

The editor and a project also built with OpenGLES, but the game didn't launch. It is likely Stride is generally not setup to work with OpenGLES on Windows/Desktop, or my graphics driver doesn't support OpenGLES.

@MaximilianEmel
Copy link
Contributor Author

MaximilianEmel commented Nov 2, 2023

I believe there's still work to be done (some visible differences in the rendering between D3D11 and OpenGL), but I haven't checked if that's been introduced from the Silk upgrade, or was always like that. There's also quite a lot of Deprecated things, that while still continue to function, ideally should be updated at some point.

@MaximilianEmel
Copy link
Contributor Author

MaximilianEmel commented Nov 2, 2023

Results from testing against 83fdc04 (last working version of OpenGL on Windows):

  • The rendering differences are the same.
  • The same error still occurs apon launching the game with OpenGLES on Windows.

Conclusions:

  • No rendering issues are originating as a result of the Silk.NET upgrade.
  • OpenGLES still needs to be tested for its target platforms, though I believe it should work.

@@ -12,22 +12,22 @@ namespace Silk.NET.OpenGL
internal class GLCoreLibraryNameContainer : SearchPathContainer
{
/// <inheritdoc />
public override string Linux => "libGL.so.1";
public override string[] Linux => new[] { "libGL.so.1" };
Copy link
Member

Choose a reason for hiding this comment

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

Can you make those properties { get; } = instead of => to avoid per call allocations of the array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into this as soon as I can.

Copy link
Contributor Author

@MaximilianEmel MaximilianEmel Nov 2, 2023

Choose a reason for hiding this comment

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

I modeled how I did it based on how Silk.NET internally does the same thing (should we do a pull to Silk to change this?): e.g. https://github.com/dotnet/Silk.NET/blob/da7cae57614f91d0387fa51af8a52c99b7c43d98/src/Windowing/Silk.NET.SDL/SDLLibraryNameContainer.cs

Copy link
Contributor Author

@MaximilianEmel MaximilianEmel Nov 2, 2023

Choose a reason for hiding this comment

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

In fact, does it even make sense in the first place to have this file here, instead of just the one already in Silk.NET? See: https://github.com/dotnet/Silk.NET/blob/da7cae57614f91d0387fa51af8a52c99b7c43d98/src/OpenGL/Silk.NET.OpenGL/GLCoreLibraryNameContainer.cs

@MaximilianEmel
Copy link
Contributor Author

MaximilianEmel commented Nov 2, 2023

After I do an Android (OpenGLES) build test, it should be ready to merge.

@manio143
Copy link
Member

manio143 commented Nov 3, 2023

I see that just as there was GLCoreLibraryNameContainer there's also OpenGLESLibraryNameContainer in Stride code base. Those were introduced by @xen2 in f982b42

@MaximilianEmel
Copy link
Contributor Author

I have been modifying both.

@tebjan
Copy link
Member

tebjan commented Nov 3, 2023

Great, thanks a lot!

@tebjan tebjan merged commit c45817e into stride3d:master Nov 3, 2023
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.

3 participants