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

Test And Normalize Vertex Behavior on All Backends #4723

Merged
merged 27 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,14 @@ Bottom level categories:

## Unreleased

### New Features

#### General
- Added `DownlevelFlags::VERTEX_AND_INSTANCE_INDEX_RESPECTS_RESPECTIVE_INDIRECT_FIRST` to know if `@builtin(vertex_index)` and `@builtin(instance_index)` will respect the base vertex / base instance in indirect calls. If this is not present, both will always start counting from 0. Currently enabled on all backends except DX12. By @cwfitzgerald in [#4722](https://github.com/gfx-rs/wgpu/pull/4722)

#### OpenGL
- `@builtin(instance_index)` now properly reflects the range provided in the draw call instead of always counting from 0. By @cwfitzgerald in [#4722](https://github.com/gfx-rs/wgpu/pull/4722).

### Changes

- Arcanization of wgpu core resources:
Expand All @@ -65,6 +73,7 @@ By @gents83 in [#3626](https://github.com/gfx-rs/wgpu/pull/3626) and tnx also to

- Log vulkan validation layer messages during instance creation and destruction: By @exrook in [#4586](https://github.com/gfx-rs/wgpu/pull/4586)
- `TextureFormat::block_size` is deprecated, use `TextureFormat::block_copy_size` instead: By @wumpf in [#4647](https://github.com/gfx-rs/wgpu/pull/4647)
- Rename of `DispatchIndirect`, `DrawIndexedIndirect`, and `DrawIndirect` types in the `wgpu::util` module to `DispatchIndirectArgs`, `DrawIndexedIndirectArgs`, and `DrawIndirectArgs`. By @cwfitzgerald in [#4723](https://github.com/gfx-rs/wgpu/pull/4723).

#### Safe `Surface` creation

Expand Down
1 change: 1 addition & 0 deletions naga/src/back/glsl/keywords.rs
Original file line number Diff line number Diff line change
Expand Up @@ -480,4 +480,5 @@ pub const RESERVED_KEYWORDS: &[&str] = &[
// Naga utilities:
super::MODF_FUNCTION,
super::FREXP_FUNCTION,
super::BASE_INSTANCE_BINDING,
];
113 changes: 82 additions & 31 deletions naga/src/back/glsl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ const CLAMPED_LOD_SUFFIX: &str = "_clamped_lod";
pub(crate) const MODF_FUNCTION: &str = "naga_modf";
pub(crate) const FREXP_FUNCTION: &str = "naga_frexp";

// Must match code in glsl_built_in
pub const BASE_INSTANCE_BINDING: &str = "naga_vs_base_instance";
cwfitzgerald marked this conversation as resolved.
Show resolved Hide resolved

/// Mapping between resources and bindings.
pub type BindingMap = std::collections::BTreeMap<crate::ResourceBinding, u8>;

Expand Down Expand Up @@ -235,17 +238,20 @@ bitflags::bitflags! {
/// Supports GL_EXT_texture_shadow_lod on the host, which provides
/// additional functions on shadows and arrays of shadows.
const TEXTURE_SHADOW_LOD = 0x2;
/// Supports ARB_shader_draw_parameters on the host, which provides
/// support for `gl_BaseInstanceARB`, `gl_BaseVertexARB`, and `gl_DrawIDARB`.
const DRAW_PARAMETERS = 0x4;
/// Include unused global variables, constants and functions. By default the output will exclude
/// global variables that are not used in the specified entrypoint (including indirect use),
/// all constant declarations, and functions that use excluded global variables.
const INCLUDE_UNUSED_ITEMS = 0x4;
const INCLUDE_UNUSED_ITEMS = 0x10;
/// Emit `PointSize` output builtin to vertex shaders, which is
/// required for drawing with `PointList` topology.
///
/// https://registry.khronos.org/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.html#built-in-language-variables
/// The variable gl_PointSize is intended for a shader to write the size of the point to be rasterized. It is measured in pixels.
/// If gl_PointSize is not written to, its value is undefined in subsequent pipe stages.
const FORCE_POINT_SIZE = 0x10;
const FORCE_POINT_SIZE = 0x20;
}
}

Expand Down Expand Up @@ -388,6 +394,24 @@ impl IdGenerator {
}
}

/// Assorted options needed for generting varyings.
#[derive(Clone, Copy)]
struct VaryingOptions {
output: bool,
targetting_webgl: bool,
draw_parameters: bool,
}

impl VaryingOptions {
const fn from_writer_options(options: &Options, output: bool) -> Self {
Self {
output,
targetting_webgl: options.version.is_webgl(),
draw_parameters: options.writer_flags.contains(WriterFlags::DRAW_PARAMETERS),
}
}
}

/// Helper wrapper used to get a name for a varying
///
/// Varying have different naming schemes depending on their binding:
Expand All @@ -398,8 +422,7 @@ impl IdGenerator {
struct VaryingName<'a> {
binding: &'a crate::Binding,
stage: ShaderStage,
output: bool,
targetting_webgl: bool,
options: VaryingOptions,
}
impl fmt::Display for VaryingName<'_> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Expand All @@ -411,7 +434,7 @@ impl fmt::Display for VaryingName<'_> {
write!(f, "_fs2p_location1",)
}
crate::Binding::Location { location, .. } => {
let prefix = match (self.stage, self.output) {
let prefix = match (self.stage, self.options.output) {
(ShaderStage::Compute, _) => unreachable!(),
// pipeline to vertex
(ShaderStage::Vertex, false) => "p2vs",
Expand All @@ -423,11 +446,7 @@ impl fmt::Display for VaryingName<'_> {
write!(f, "_{prefix}_location{location}",)
}
crate::Binding::BuiltIn(built_in) => {
write!(
f,
"{}",
glsl_built_in(built_in, self.output, self.targetting_webgl)
)
write!(f, "{}", glsl_built_in(built_in, self.options))
}
}
}
Expand Down Expand Up @@ -569,7 +588,11 @@ impl<'a, W: Write> Writer<'a, W> {
keywords::RESERVED_KEYWORDS,
&[],
&[],
&["gl_"],
&[
"gl_", // all GL built-in variables
"_group", // all normal bindings
"_push_constant_binding_", // all push constant bindings
],
&mut names,
);

Expand Down Expand Up @@ -632,6 +655,17 @@ impl<'a, W: Write> Writer<'a, W> {
// https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_shadow_lod.txt
writeln!(self.out, "#extension GL_EXT_texture_shadow_lod : require")?;
}
if self
.options
.writer_flags
.contains(WriterFlags::DRAW_PARAMETERS)
{
// https://registry.khronos.org/OpenGL/extensions/ARB/ARB_shader_draw_parameters.txt
writeln!(
self.out,
"#extension GL_ARB_shader_draw_parameters : require"
)?;
}

// glsl es requires a precision to be specified for floats and ints
// TODO: Should this be user configurable?
Expand All @@ -652,6 +686,16 @@ impl<'a, W: Write> Writer<'a, W> {
writeln!(self.out)?;
}

if self.entry_point.stage == ShaderStage::Vertex
&& !self
.options
.writer_flags
.contains(WriterFlags::DRAW_PARAMETERS)
{
writeln!(self.out, "uniform uint {BASE_INSTANCE_BINDING};")?;
cwfitzgerald marked this conversation as resolved.
Show resolved Hide resolved
writeln!(self.out)?;
}

// Enable early depth tests if needed
if let Some(depth_test) = self.entry_point.early_depth_test {
// If early depth test is supported for this version of GLSL
Expand Down Expand Up @@ -1422,7 +1466,10 @@ impl<'a, W: Write> Writer<'a, W> {
writeln!(
self.out,
"invariant {};",
glsl_built_in(built_in, output, self.options.version.is_webgl())
glsl_built_in(
built_in,
VaryingOptions::from_writer_options(self.options, output)
)
)?;
}
}
Expand Down Expand Up @@ -1499,8 +1546,7 @@ impl<'a, W: Write> Writer<'a, W> {
second_blend_source,
},
stage: self.entry_point.stage,
output,
targetting_webgl: self.options.version.is_webgl(),
options: VaryingOptions::from_writer_options(self.options, output),
};
writeln!(self.out, " {vname};")?;

Expand Down Expand Up @@ -1661,8 +1707,7 @@ impl<'a, W: Write> Writer<'a, W> {
let varying_name = VaryingName {
binding: member.binding.as_ref().unwrap(),
stage,
output: false,
targetting_webgl: self.options.version.is_webgl(),
options: VaryingOptions::from_writer_options(self.options, false),
};
if index != 0 {
write!(self.out, ", ")?;
Expand All @@ -1675,8 +1720,7 @@ impl<'a, W: Write> Writer<'a, W> {
let varying_name = VaryingName {
binding: arg.binding.as_ref().unwrap(),
stage,
output: false,
targetting_webgl: self.options.version.is_webgl(),
options: VaryingOptions::from_writer_options(self.options, false),
};
writeln!(self.out, "{varying_name};")?;
}
Expand Down Expand Up @@ -2170,8 +2214,10 @@ impl<'a, W: Write> Writer<'a, W> {
let varying_name = VaryingName {
binding: member.binding.as_ref().unwrap(),
stage: ep.stage,
output: true,
targetting_webgl: self.options.version.is_webgl(),
options: VaryingOptions::from_writer_options(
self.options,
true,
),
};
write!(self.out, "{varying_name} = ")?;

Expand All @@ -2195,8 +2241,10 @@ impl<'a, W: Write> Writer<'a, W> {
let name = VaryingName {
binding: result.binding.as_ref().unwrap(),
stage: ep.stage,
output: true,
targetting_webgl: self.options.version.is_webgl(),
options: VaryingOptions::from_writer_options(
self.options,
true,
),
};
write!(self.out, "{name} = ")?;
self.write_expr(value, ctx)?;
Expand Down Expand Up @@ -4308,29 +4356,32 @@ const fn glsl_scalar(scalar: crate::Scalar) -> Result<ScalarString<'static>, Err
}

/// Helper function that returns the glsl variable name for a builtin
const fn glsl_built_in(
built_in: crate::BuiltIn,
output: bool,
targetting_webgl: bool,
) -> &'static str {
const fn glsl_built_in(built_in: crate::BuiltIn, options: VaryingOptions) -> &'static str {
use crate::BuiltIn as Bi;

match built_in {
Bi::Position { .. } => {
if output {
if options.output {
"gl_Position"
} else {
"gl_FragCoord"
}
}
Bi::ViewIndex if targetting_webgl => "int(gl_ViewID_OVR)",
Bi::ViewIndex if options.targetting_webgl => "int(gl_ViewID_OVR)",
Bi::ViewIndex => "gl_ViewIndex",
// vertex
Bi::BaseInstance => "uint(gl_BaseInstance)",
Bi::BaseVertex => "uint(gl_BaseVertex)",
Bi::ClipDistance => "gl_ClipDistance",
Bi::CullDistance => "gl_CullDistance",
Bi::InstanceIndex => "uint(gl_InstanceID)",
Bi::InstanceIndex => {
if options.draw_parameters {
"(uint(gl_InstanceID) + uint(gl_BaseInstanceARB))"
cwfitzgerald marked this conversation as resolved.
Show resolved Hide resolved
} else {
// Must match BASE_INSTANCE_BINDING
"(uint(gl_InstanceID) + naga_vs_base_instance)"
}
}
Bi::PointSize => "gl_PointSize",
Bi::VertexIndex => "uint(gl_VertexID)",
// fragment
Expand All @@ -4340,7 +4391,7 @@ const fn glsl_built_in(
Bi::PrimitiveIndex => "uint(gl_PrimitiveID)",
Bi::SampleIndex => "gl_SampleID",
Bi::SampleMask => {
if output {
if options.output {
"gl_SampleMask"
} else {
"gl_SampleMaskIn"
Expand Down
3 changes: 2 additions & 1 deletion naga/tests/in/push-constants.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ struct FragmentIn {
@vertex
fn vert_main(
@location(0) pos : vec2<f32>,
@builtin(instance_index) ii: u32,
@builtin(vertex_index) vi: u32,
) -> @builtin(position) vec4<f32> {
return vec4<f32>(f32(vi) * pc.multiplier * pos, 0.0, 1.0);
return vec4<f32>(f32(ii) * f32(vi) * pc.multiplier * pos, 0.0, 1.0);
}

@fragment
Expand Down
2 changes: 2 additions & 0 deletions naga/tests/out/glsl/access.foo_vert.Vertex.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

struct GlobalConst {
uint a;
uvec3 b;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;


void main() {
uint in_vertex_index = uint(gl_VertexID);
Expand Down
2 changes: 2 additions & 0 deletions naga/tests/out/glsl/image.queries.Vertex.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

uniform highp sampler2D _group_0_binding_0_vs;

uniform highp sampler2D _group_0_binding_1_vs;
Expand Down
2 changes: 2 additions & 0 deletions naga/tests/out/glsl/interpolate.vert_main.Vertex.glsl
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#version 400 core
uniform uint naga_vs_base_instance;

struct FragmentInput {
vec4 position;
uint _flat;
Expand Down
2 changes: 2 additions & 0 deletions naga/tests/out/glsl/invariant.vs.Vertex.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

invariant gl_Position;

void main() {
Expand Down
2 changes: 2 additions & 0 deletions naga/tests/out/glsl/padding.vertex.Vertex.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

struct S {
vec3 a;
};
Expand Down
7 changes: 5 additions & 2 deletions naga/tests/out/glsl/push-constants.vert_main.Vertex.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

struct PushConstants {
float multiplier;
};
Expand All @@ -15,9 +17,10 @@ layout(location = 0) in vec2 _p2vs_location0;

void main() {
vec2 pos = _p2vs_location0;
uint ii = (uint(gl_InstanceID) + naga_vs_base_instance);
uint vi = uint(gl_VertexID);
float _e5 = _push_constant_binding_vs.multiplier;
gl_Position = vec4(((float(vi) * _e5) * pos), 0.0, 1.0);
float _e8 = _push_constant_binding_vs.multiplier;
gl_Position = vec4((((float(ii) * float(vi)) * _e8) * pos), 0.0, 1.0);
return;
}

2 changes: 2 additions & 0 deletions naga/tests/out/glsl/quad-vert.main.Vertex.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

struct gen_gl_PerVertex {
vec4 gen_gl_Position;
float gen_gl_PointSize;
Expand Down
2 changes: 2 additions & 0 deletions naga/tests/out/glsl/quad.vert_main.Vertex.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

struct VertexOutput {
vec2 uv;
vec4 position;
Expand Down
2 changes: 2 additions & 0 deletions naga/tests/out/glsl/shadow.vs_main.Vertex.glsl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

struct Globals {
mat4x4 view_proj;
uvec4 num_lights;
Expand Down
Loading
Loading