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

expose webgl extensions through parameter api #165

Merged
merged 5 commits into from
Jun 17, 2021

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Jun 11, 2021

Currently it is impossible to query for webgl extensions, due to it using specialized unexposed api for retreiving the list. This PR exposes the list with desktop gl api by intercepting parameter calls and implementing them using the web-specific api under the hood.

@Gordon-F
Copy link
Contributor

I think it would be better to add utility api, for example, get_supported_extensions where glow gets a number of extensions and return it to users. And it can be implemented without any extra specific if/else inside other functions.

@Frizi
Copy link
Contributor Author

Frizi commented Jun 11, 2021

I agree that some non-standard api that does the thing would likely be better overall. Still, this approach allows us to just piggyback on existing standard, so you don't need any extra platform-specific code on the other side.

@grovesNL
Copy link
Owner

Thanks!

I like @Gordon-F's idea to add a non-standard API here: when targeting natively -- most projects will end up splitting the string again or searching for an extension name in the string. which could be simplified if we can expose Vec<String> here instead. We've diverged from the spec a little bit when it improves ergonomics a bit so it seems ok to do it here.

For the specific case in gfx-rs/gfx#3773 it would also be nice to have functions that follow the existing supports_xyz-style (e.g. fn supports_texture_compression_s3tc() -> bool, so we could internally check GL_EXT_texture_compression_s3tc on OpenGL and WEBGL_compressed_texture_s3tc on WebGL, which avoids target-specific extension name strings.

@Frizi
Copy link
Contributor Author

Frizi commented Jun 12, 2021

I've added dedicated get_supported_extensions method, and also made sure to use older api to retrieve that list in old GL versions. To do that, I've included the GL and GLSL version parsing code from gfx. That means we can get rid of it downstream.

Adding the "supports_x" methods is more tricky and I don't exactly want to go there inside glow's scope yet. This is complicated, because different sets of extensions (which are in core from some versions) are responsible for that in different platforms. Also the feature is kinda split into two: no-srgb and srgb texture. I only care about webgl right now for this particular case, so I haven't fully explored what's the proper set of checks necessary for portability. But the gist of it is that there are two extensions on webgl, one extension for basic support, and a whole set of interedependent extensions for srgb part on native gl. Also some of those are in core in different versions, so that would also have to be checked. We already have the right abstraction to check those in gfx gl backend, and a lot of that would have to be replicated in glow. I'm not sure if it's worth it.

Copy link
Owner

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! I had a couple minor comments

src/native.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Owner

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

Nice, thank you!
bors r+

@bors
Copy link
Contributor

bors bot commented Jun 17, 2021

@bors bors bot merged commit 23d1627 into grovesNL:main Jun 17, 2021
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