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

Support wgpu skybox example on webgl #3773

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Frizi
Copy link
Contributor

@Frizi Frizi commented Jun 11, 2021

This changesets adds necessary features to GL backend in order to support skybox example from wgpu repository in webgl.

Most notable is adding support for cubemaps. Unfortunately due to the complexity of emulating image views in GL and the fact that you can't use a single gl texture in multiple ways, the support is very limited. Every 6-layer square image is assumed to be a cubemap, and creating other than cubemap views to it will fail. Further work on that should be done eventually, but I want to get the basic support first.

Another thing is support for compressed textures, specifically S3TC formats. This was added, because skybox example texture format fallback ise BGRA, which is not supported on webgl at all. Adding support for compressed format allowed me to run the example unmodified. Further work here is adding more formats and adding a downlevel flag for BGRA texture format, which is for some reason not supported on GLES.

@@ -689,6 +690,13 @@ pub(crate) fn query_all(
{
features |= Features::INDEPENDENT_BLENDING;
}
if info.is_supported(&[
Es(3, 0),
Ext("WEBGL_compressed_texture_s3tc"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, extensions list is empty for Webgl.

let extensions = if crate::is_webgl() {
HashSet::new()
} else if (version >= Version::new(3, 0, None, String::from("")))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. It worked for me because the is_supported is an "or" of passed requirements, and Es 3.0 is always satisfied. That means this code is indeed wrong. I need to populate the extension list.

Copy link
Contributor

Choose a reason for hiding this comment

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

First we need to expose extension list from glow somehow. Or copy-paste extensions checking from glow codebase:
https://github.com/grovesNL/glow/blob/7775e74363877347b2210d91277646a09d70dd08/src/web_sys.rs#L105-L109

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've exposed the extensions in grovesNL/glow#165. Let's wait for it to be in.

@@ -115,7 +115,7 @@ impl Queue {
(glow::BYTE, glow::SHORT, glow::INT),
C::Uint | C::Unorm =>
(glow::UNSIGNED_BYTE, glow::UNSIGNED_SHORT, glow::UNSIGNED_INT),
C::Float => (glow::ZERO, glow::HALF_FLOAT, glow::FLOAT),
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidentally removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, though the commented out code shouldn't really be here in the first place. We have git for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be better to remove all commented code?

@@ -87,6 +76,70 @@ impl FormatDescription {
}
}

pub const COMPRESSED_RGB_S3TC_DXT1_EXT: u32 = 0x83F0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to expose this on glow side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 opened a PR grovesNL/glow#166

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