[glsl-out] split reflection_names into a map for types and another for globals #1144
Conversation
|
This fixes the WGPU Bevy problem I ran into! ❤️ Thanks! |
src/back/glsl/mod.rs
Outdated
| @@ -312,7 +312,9 @@ pub struct Writer<'a, W> { | |||
| /// (generated by a [`Namer`](crate::proc::Namer)) | |||
| names: crate::FastHashMap<NameKey, String>, | |||
| /// A map with all the names needed for reflections | |||
| reflection_names: crate::FastHashMap<Handle<crate::Type>, String>, | |||
| reflection_names_structs: crate::FastHashMap<Handle<crate::Type>, String>, | |||
There was a problem hiding this comment.
Maybe reflection_names_buffers or reflection_names_uniforms?
There was a problem hiding this comment.
why do we ever need to reflect the type names? they shouldn't be exposed anywhere in the GL API
There was a problem hiding this comment.
We need names only for uniforms here - https://github.com/gfx-rs/wgpu/blob/master/wgpu-hal/src/gles/device.rs#L38.
they shouldn't be exposed anywhere in the GL API
Performance? We already got this info.
There was a problem hiding this comment.
I renamed it to reflection_names_uniforms.
There was a problem hiding this comment.
@Gordon-F that lookup goes with a variable handle, not a type handle. I'm asking why we need to look up any reflection based on the type.
| @@ -2455,7 +2457,7 @@ impl<'a, W: Write> Writer<'a, W> { | |||
| match self.module.types[var.ty].inner { | |||
| crate::TypeInner::Struct { .. } => match var.class { | |||
| crate::StorageClass::Uniform | crate::StorageClass::Storage => { | |||
| let name = self.reflection_names[&var.ty].clone(); | |||
| let name = self.reflection_names_uniforms[&var.ty].clone(); | |||
There was a problem hiding this comment.
why are we looking up var.ty instead of looking up handle directly in that other map?
There was a problem hiding this comment.
I believe this is because the write_struct function, where reflection_names_uniforms.insert is called, has no access to the global variable handle, only the type handle.
There was a problem hiding this comment.
Yep, we need a block struct name, that will be generated as uniform buffer.
Lines 1233 to 1239 in ab2fa43
There was a problem hiding this comment.
If we will use the global variable handle here, we still need to check the variable type and ensure that the variable is a struct with a uniform storage class.
If we want to change this, we need to change block struct writing.
There was a problem hiding this comment.
ok I see. So the block name shows up in the API, not the variable name, and the block name comes from the type. Ok
jakobhellermann
force-pushed
the
fix-duplicate-sampler
branch
from
ab2fa43
to
b7364b0
Compare
jakobhellermann
force-pushed
the
fix-duplicate-sampler
branch
from
b7364b0
to
d809e2f
Compare
I rebased the PR. |
…r globals (gfx-rs#1144) * split reflection_names into a map for types and another for globals * reflection_names_structs -> reflection_names_uniforms
fixes #1137, not sure whether this is the right approach
The issue in #1137 was that the
reflection_namesmap was per type but different globals shared the same type, so the first one got overwritten.This PR splits the
reflection_namesinto a map ofHandle<GlobalVariable> => Stringand the oldHandle<Type> => String.I'm not sure if the globals map is even needed though, because the usage here could just be
let tex_name = self.get_global_name(handle, global)and the global map could be removed.The last change to this code was in #606, @Gordon-F can you take a look and comment on which approach you think is best?