Skip to content

Commit

Permalink
Major fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@cwfitzgerald
cwfitzgerald committed Nov 20, 2023
1 parent 23c3d23 commit 37eb770
Show file tree
Hide file tree
Showing 11 changed files with 250 additions and 118 deletions.
102 changes: 70 additions & 32 deletions naga/src/back/glsl/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,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 @@ -391,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 @@ -401,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 @@ -414,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 @@ -426,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 @@ -639,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 @@ -659,7 +686,12 @@ impl<'a, W: Write> Writer<'a, W> {
writeln!(self.out)?;
}

if self.entry_point.stage == ShaderStage::Vertex {
if self.entry_point.stage == ShaderStage::Vertex
&& !self
.options
.writer_flags
.contains(WriterFlags::DRAW_PARAMETERS)
{
writeln!(self.out, "uniform uint {BASE_INSTANCE_BINDING};")?;
writeln!(self.out)?;
}
Expand Down Expand Up @@ -1434,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 @@ -1511,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 @@ -1673,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 @@ -1687,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 @@ -2182,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 @@ -2207,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 @@ -4320,30 +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",
// Must match BASE_INSTANCE_BINDING
Bi::InstanceIndex => "(uint(gl_InstanceID) + naga_vs_base_instance)",
Bi::InstanceIndex => {
if options.draw_parameters {
"(uint(gl_InstanceID) + uint(gl_BaseInstanceARB))"
} 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 @@ -4353,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
7 changes: 2 additions & 5 deletions naga/tests/in/push-constants.param.ron
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
(
god_mode: true,
glsl: (
version: Embedded(
version: 320,
is_webgl: false
),
writer_flags: (""),
version: Desktop(420),
writer_flags: ("DRAW_PARAMETERS"),
binding_map: {},
zero_initialize_workgroup_memory: true,
),
Expand Down
7 changes: 2 additions & 5 deletions naga/tests/out/glsl/push-constants.main.Fragment.glsl
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
#version 320 es

precision highp float;
precision highp int;

#version 420 core
#extension GL_ARB_shader_draw_parameters : require
struct PushConstants {
float multiplier;
};
Expand Down
11 changes: 3 additions & 8 deletions naga/tests/out/glsl/push-constants.vert_main.Vertex.glsl
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
#version 320 es

precision highp float;
precision highp int;

uniform uint naga_vs_base_instance;

#version 420 core
#extension GL_ARB_shader_draw_parameters : require
struct PushConstants {
float multiplier;
};
Expand All @@ -17,7 +12,7 @@ layout(location = 0) in vec2 _p2vs_location0;

void main() {
vec2 pos = _p2vs_location0;
uint ii = (uint(gl_InstanceID) + naga_vs_base_instance);
uint ii = (uint(gl_InstanceID) + uint(gl_BaseInstanceARB));
uint vi = uint(gl_VertexID);
float _e8 = _push_constant_binding_vs.multiplier;
gl_Position = vec4((((float(ii) * float(vi)) * _e8) * pos), 0.0, 1.0);
Expand Down
45 changes: 30 additions & 15 deletions tests/tests/vertex_indices/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,23 @@ impl Draw {
}
}

fn add_to_buffer(&self, bytes: &mut Vec<u8>) {
fn add_to_buffer(&self, bytes: &mut Vec<u8>, features: wgpu::Features) {
// The behavior of non-zero first_instance in indirect draw calls in currently undefined if INDIRECT_FIRST_INSTANCE is not supported.
let supports_first_instance = features.contains(wgpu::Features::INDIRECT_FIRST_INSTANCE);
let first_instance = if supports_first_instance {
self.instance.start
} else {
0
};

if let Some(vertex_offset) = self.vertex_offset {
bytes.extend_from_slice(
wgpu::util::DrawIndexedIndirectArgs {
vertex_count: self.vertex.end - self.vertex.start,
instance_count: self.instance.end - self.instance.start,
vertex_offset,
first_index: self.vertex.start,
first_instance: self.instance.start,
first_instance,
}
.as_bytes(),
)
Expand All @@ -37,7 +45,7 @@ impl Draw {
vertex_count: self.vertex.end - self.vertex.start,
instance_count: self.instance.end - self.instance.start,
first_vertex: self.vertex.start,
first_instance: self.instance.start,
first_instance,
}
.as_bytes(),
)
Expand Down Expand Up @@ -151,38 +159,43 @@ struct Test {

impl Test {
fn expectation(&self, ctx: &TestingContext) -> &'static [u32] {
let is_indirect = matches!(self.draw_call_kind, DrawCallKind::Indirect);

// Both of these failure modes require indirect rendering

// If this is false, the first instance will be ignored.
let non_zero_first_instance_supported = ctx
.adapter
.features()
.contains(wgpu::Features::INDIRECT_FIRST_INSTANCE);
.contains(wgpu::Features::INDIRECT_FIRST_INSTANCE)
|| !is_indirect;

// If this is false, it won't be ignored, but it won't show up in the shader
//
// If the IdSource is buffers, this doesn't apply
let first_vert_instance_supported = ctx.adapter_downlevel_capabilities.flags.contains(
wgpu::DownlevelFlags::VERTEX_AND_INSTANCE_INDEX_RESPECTS_RESPECTIVE_INDIRECT_FIRST,
);

let is_indirect = matches!(self.draw_call_kind, DrawCallKind::Indirect);
) || matches!(self.id_source, IdSource::Buffers)
|| !is_indirect;

match self.case {
TestCase::DrawBaseVertex => {
if !first_vert_instance_supported && is_indirect {
if !first_vert_instance_supported {
return &[0, 1, 2, 3, 4, 5];
}

&[0, 0, 0, 3, 4, 5, 6, 7, 8]
}
TestCase::Draw => &[0, 1, 2, 3, 4, 5],
TestCase::DrawVertexOffset | TestCase::DrawInstanced => {
if !first_vert_instance_supported && is_indirect {
TestCase::Draw | TestCase::DrawInstanced => &[0, 1, 2, 3, 4, 5],
TestCase::DrawVertexOffset => {
if !first_vert_instance_supported {
return &[0, 1, 2, 0, 0, 0];
}

&[0, 1, 2, 3, 4, 5]
}
TestCase::DrawInstancedOffset => {
if (!first_vert_instance_supported || !non_zero_first_instance_supported)
&& is_indirect
{
if !first_vert_instance_supported || !non_zero_first_instance_supported {
return &[0, 1, 2, 0, 0, 0];
}

Expand Down Expand Up @@ -300,6 +313,8 @@ fn vertex_index_common(ctx: TestingContext) {
}
}

let features = ctx.adapter.features();

let mut failed = false;
for test in tests {
let pipeline = match test.id_source {
Expand Down Expand Up @@ -367,7 +382,7 @@ fn vertex_index_common(ctx: TestingContext) {
DrawCallKind::Indirect => {
let mut indirect_bytes = Vec::new();
for draw in draws {
draw.add_to_buffer(&mut indirect_bytes);
draw.add_to_buffer(&mut indirect_bytes, features);
}
indirect_buffer = ctx.device.create_buffer_init(&BufferInitDescriptor {
label: Some("indirect"),
Expand Down
Loading

0 comments on commit 37eb770

Please sign in to comment.