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

Fixes has_integer_attributes and macroquad #461

Closed
wants to merge 1 commit into from

Conversation

birhburh
Copy link
Contributor

Fixes this issue:
not-fl3/macroquad#747
Added manual shader parser because opengl doesn't have any function to get this info

@not-fl3
Copy link
Owner

not-fl3 commented Jun 24, 2024

A little context here:

#457 turned out to be quite a breaking change. Before, VertexFormat was only about CPU representation, in the sahders it was all the same. In other words, this worked:

#[repr(C)]
struct Vertex {
    pos: [f32; 2],
    color: [u8; 4],
}

let pipeline = ctx.new_pipeline(
    &[BufferLayout::default()],
    &[
        VertexAttribute::new("in_pos", VertexFormat::Float2),
        VertexAttribute::new("in_color", VertexFormat::Byte4),
    ],
    shader,
    PipelineParams::default(),
);

pub const VERTEX: &str = r#"#version 100
in lowp vec4 in_color;

And now VertexFormat is any integer, the value is being as uvec4 instead of vec4, shader version should be at least 150, a lot of old code doesn't work anymore.

miniquad intentionally do not try to be smart about shaders, leaving it for the third party libraries. I think the correct solution: add a flag for VertexAttribute that its a integer-integer, or add a VertexFormat::IByte4 or something, indicating that those values will be passed to shaders as integers.

I do not really know, can't build an opinion yet, for now I just yanked miniquad 0.4.3

cc @eloraiby

@not-fl3
Copy link
Owner

not-fl3 commented Jun 24, 2024

A quick hack to illustrate the idea:

What I like:

  • if you want to take responsibility on handling shader's minimal version, you can still do it and benefit from uvec4
  • it is backward compatible, old code keep works just as it used to be
  • on metal, apparently, you can receive it as either uint4 or float4

What I don't like:

  • naming, gl_pass_as_float is a weird flag

Maybe I am missing something and there is more to think about here?

diff --git a/examples/triangle_color4b.rs b/examples/triangle_color4b.rs
index f06a302..e01a3c3 100644
--- a/examples/triangle_color4b.rs
+++ b/examples/triangle_color4b.rs
@@ -60,7 +60,10 @@ impl Stage {
             &[BufferLayout::default()],
             &[
                 VertexAttribute::new("in_pos", VertexFormat::Float2),
-                VertexAttribute::new("in_color", VertexFormat::Byte4),
+                VertexAttribute {
+                    gl_pass_as_float: false,
+                    ..VertexAttribute::new("in_color", VertexFormat::Byte4)
+                },
             ],
             shader,
             PipelineParams::default(),
diff --git a/src/graphics.rs b/src/graphics.rs
index e9e8e2b..b0cd117 100644
--- a/src/graphics.rs
+++ b/src/graphics.rs
@@ -236,6 +236,7 @@ pub struct VertexAttribute {
     pub name: &'static str,
     pub format: VertexFormat,
     pub buffer_index: usize,
+    pub gl_pass_as_float: bool,
 }
 
 impl VertexAttribute {
@@ -252,6 +253,7 @@ impl VertexAttribute {
             name,
             format,
             buffer_index,
+            gl_pass_as_float: true,
         }
     }
 }
diff --git a/src/graphics/gl.rs b/src/graphics/gl.rs
index 9bfe01e..f0136d1 100644
--- a/src/graphics/gl.rs
+++ b/src/graphics/gl.rs
@@ -1070,6 +1070,7 @@ impl RenderingBackend for GlContext {
             name,
             format,
             buffer_index,
+            gl_pass_as_float,
         } in attributes
         {
             let buffer_data = &mut buffer_cache
@@ -1105,6 +1106,7 @@ impl RenderingBackend for GlContext {
                         stride: buffer_data.stride,
                         buffer_index: *buffer_index,
                         divisor,
+                        gl_pass_as_float: *gl_pass_as_float,
                     };
 
                     assert!(
@@ -1332,7 +1334,10 @@ impl RenderingBackend for GlContext {
                     unsafe {
                         match attribute.type_ {
                             GL_INT | GL_UNSIGNED_INT | GL_SHORT | GL_UNSIGNED_SHORT
-                            | GL_UNSIGNED_BYTE | GL_BYTE => {
+                            | GL_UNSIGNED_BYTE | GL_BYTE
+                                if !attribute.gl_pass_as_float =>
+                            {
                                 assert!(self.has_integer_attributes());
                                 glVertexAttribIPointer(
                                     attr_index as GLuint,
diff --git a/src/graphics/gl/cache.rs b/src/graphics/gl/cache.rs
index 6dd548e..3e252d9 100644
--- a/src/graphics/gl/cache.rs
+++ b/src/graphics/gl/cache.rs
@@ -9,6 +9,7 @@ pub struct VertexAttributeInternal {
     pub stride: i32,
     pub buffer_index: usize,
     pub divisor: i32,
+    pub gl_pass_as_float: bool,
 }
 
 #[derive(Default, Copy, Clone)]

@eloraiby
Copy link
Contributor

I have been thinking about the best way to approach different platforms and compatibility issues. This is for the future, but worth starting the discussion so we can take it in a specific direction.

Should we still support GLES2/WEBGL1? Most systems (96% of android devices at least) support GLES 3.0 and Firefox/Chrome/Safari support WEBGL2. Why support opengl on Mac/IOS? OpenGL ES is deprecated on IOS and OpenGL is stuck to 3.2 core, IMHO, let's just stick with Metal. On Linux, it's the wild west, but most recent systems (10 years old) should support OpenGL 3.2 (shader version 150). Windows depends on the driver specific and all drivers should support opengl 3.2 at least. Maybe have D3D 11 backend? I know that it looks more and more like sokol and we probably end up having to write some shader compilation tool as @floooh did (luckily we have naga for rust).

Pros: simplify and remove code and make it consistent :)
Cons: We propably need to fork miniquad and start a miniquad 2.

Otherwise, @not-fl3 suggestion looks like a good compromise for now.

@not-fl3
Copy link
Owner

not-fl3 commented Jun 24, 2024

Webgl1 is still a thing on old-ish iphone/safari, gl2 is still a thing on virtual machines/computers with missconfigured gpu drivers or just old computers. Its probably no aligned with general rust philosophy, but I am not really into deprecating old, but totally usable hardware for no good reason.

For OpenGL on Mac - its a very valid point, just two reasons:

  • miniqad's OpenGL backend is way more tested that Metal. Having a fallback is nice.
  • there is no shader cross compiler that works out of the box with miniquad (well, except shadermagic which is a very questionable cross compiler), so for a lot of cases glsl just works

@eloraiby
Copy link
Contributor

All what you say is valid, and that's why I think we should not shy from creating a new miniquad 2, and keep miniquad 1 to address old devices specific needs.

@birhburh
Copy link
Contributor Author

So this thread triggered my curiosity about webgl support and now i want to test miniquad on kaios
But yeah, probably closing PR for now?

Resources:

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