gfx-rs / gfx

[maintenance mode] A low-overhead Vulkan-like GPU API for Rust.
http://gfx-rs.github.io/
Apache License 2.0
5.35k stars 548 forks source link

[Wasm/WebGL] Hello-triangle panics at adapter creation. #3644

Closed VincentJousse closed 3 years ago

VincentJousse commented 3 years ago

On wasm/WebGL, the hello-triangle panics at adapter creation. I’m using wgpu-rs 0.7

grovesNL commented 3 years ago

Could you include the error you get during adapter creation?

VincentJousse commented 3 years ago

panicked at 'Error querying info: InvalidEnum', /Users/vincent/.cargo/registry/src/github.com-1ecc6299db9ec823/gfx-backend-gl-0.7.0/src/lib.rs:442:13

VincentJousse commented 3 years ago

@grovesNL should I move this in gfx issues ?

kvark commented 3 years ago

We can move it in-between repos, you don't need to worry about this.

Gordon-F commented 3 years ago

The main problem here that the version checker thinks that WebGL 2.0 == OpenGL ES 3.2 (or other ES version). I can make a PR with custom PartialEq and PartialOrd impl for Version or we can rework capabilities checking for WebGL.

kvark commented 3 years ago

WebGL2 should correspond to OpenGL ES 3.0, in which case we don't need a special implementation, I think

Gordon-F commented 3 years ago

@kvark Technically yes, but not in the current code.

For example, the this check is true for wasm code, which is wrong:

if info.is_supported(&[Core(4, 3), Es(3, 2), Ext("GL_ARB_compute_shader")]) {
}

https://github.com/gfx-rs/gfx/blob/master/src/backend/gl/src/info.rs#L402

kvark commented 3 years ago

Why is this check true? it requires Es(3,2), which WASM target doesn't have

Gordon-F commented 3 years ago

@kvark Sorry, I didn't provide complete information. Let me explain.

Let's see what is_supported do:

pub fn is_supported(&self, requirements: &[Requirement]) -> bool {
        use self::Requirement::*;
        requirements.iter().any(|r| match *r {
            Core(major, minor) => self.is_version_supported(major, minor),
            Es(major, minor) => self.is_embedded_version_supported(major, minor),
            Ext(extension) => self.is_extension_supported(extension),
        })
    }

Add extra logs to is_embedded_version_supported (original version):

pub fn is_embedded_version_supported(&self, major: u32, minor: u32) -> bool {
        let is_embedded_version_supported = self.version.is_embedded
            && self.version >= Version::new(major, minor, None, String::from(""));
        log::debug!(
            "is_embedded_version_supported = {}. Check version = {}.{}. Current version = {}.{}",
            is_embedded_version_supported,
            major,
            minor,
            self.version.major,
            self.version.minor
        );

        is_embedded_version_supported
    }

Run wgpu hello-triangle example:

hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.1. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.2. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.2. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 2.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.0. Current version = 2.0
hello-triangle.js:1182 is_embedded_version_supported = true. Check version = 3.1. Current version = 2.0

As we can see is_embedded_version_supported always true for WebGL.

Add custom PartialEq and PartialOrd impl for Version:

impl PartialEq for Version {
    fn eq(&self, other: &Self) -> bool {
        self.is_embedded == other.is_embedded && self.major == other.major && self.minor == other.minor
    }
}

impl PartialOrd for Version {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        if self.major == other.major && self.minor == self.minor {
            Some(Ordering::Equal)
        } else if self.major > other.major {
            Some(Ordering::Greater)
        } else if self.major < other.major {
            Some(Ordering::Less)
        } else if self.major == other.major && self.minor > other.minor {
            Some(Ordering::Greater)
        } else if self.major == other.major && self.minor < other.minor {
            Some(Ordering::Less)
        } else {
            None
        }
    }
}

All works fine:

webgl_triangle
kvark commented 3 years ago

Do we know why the comparison is turning true, exactly? The derived PartialOrd should compare the major then minor versions (see playground), there shouldn't be need to custom-derive it.

Gordon-F commented 3 years ago

Ohhh I think all simple. is_embedded_version_supported should be:

pub fn is_embedded_version_supported(&self, major: u32, minor: u32) -> bool {
        self.version.is_embedded && self.version >= Version::new_embedded(major, minor, None, String::from(""))
    }

We should check embedded version only with embedded version, because first bool is_embedded in Version struct.

See playground.

kvark commented 3 years ago

That makes sense, yeah!

VincentJousse commented 3 years ago

Could this land on 0.7 branch ?

kvark commented 3 years ago

@VincentFTS the GL backend has a lot of rough edges right now. This blocker is resolved, but others are still there, so even if you could use wgpu-0.7 for generating WebGL, you should heavily consider master branch instead. We'll try to release a patch to gfx-backend-gl, fwiw.

kvark commented 3 years ago

Thanks to @Gordon-F quick followup, you can now get the fix by doing cargo update -p gfx-backend-gl