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

Shader Sampler2D Uniforms are always listed last in the Inspector #75454

Closed
fracteed opened this issue Mar 29, 2023 · 2 comments · Fixed by #94785
Closed

Shader Sampler2D Uniforms are always listed last in the Inspector #75454

fracteed opened this issue Mar 29, 2023 · 2 comments · Fixed by #94785

Comments

@fracteed
Copy link

Godot version

4.0 stable

System information

Windows 10 960m

Issue description

When creating sampler2d uniforms in a shader, they get listed at the bottom of the uniform group in the inspector. It is often useful to have them listed with related uniforms, so they should ideally follow the same order as listed in the shader code. For instance, albedo texture followed by the albedo color.

This example shows the standard 3d material converted to a shader with some small edits for readability. Note that the 3 sampler 2d uniforms are displayed at the bottom in the inspector and not in the shader order. However, they do display in the relative order that they appear in the shader code.

sampler2d_order

@Chaosus thanks for your fix on the alphabetical uniform group ordering! I thought it would be a good time to add this issue as the final obstacle to getting the perfect uniform ordering. Pretty sure this same issue also happened in 3.x and is a long standing problem.

Steps to reproduce

  • create a standard 3d material
  • convert this material to a shader
  • note that all of the uniform sample2d are listed at the bottom in the outliner

Minimal reproduction project

sampler2d_order.zip

@Chaosus
Copy link
Member

Chaosus commented Mar 29, 2023

I think this is intended, and sampler uniforms are indexed, 100000 after the common uniforms in the 3.x and now. Here, I just modified the old code to make it works as before:

void MaterialStorage::ShaderData::get_shader_uniform_list(List<PropertyInfo> *p_param_list) const {
SortArray<Pair<StringName, int>, ShaderLanguage::UniformOrderComparator> sorter;
LocalVector<Pair<StringName, int>> filtered_uniforms;
for (const KeyValue<StringName, ShaderLanguage::ShaderNode::Uniform> &E : uniforms) {
if (E.value.scope != ShaderLanguage::ShaderNode::Uniform::SCOPE_LOCAL) {
continue;
}
if (E.value.texture_order >= 0) {
filtered_uniforms.push_back(Pair<StringName, int>(E.key, E.value.texture_order + 100000));
} else {
filtered_uniforms.push_back(Pair<StringName, int>(E.key, E.value.order));
}
}

cc @clayjohn - what's the purpose of that comfortability, it was by your opinion?

@clayjohn
Copy link
Member

@Chaosus I didn't add this code, nor have I worked on it much.

If I had to guess I would say that reduz added it to allow for quickly parsing whether a given uniform is a texture or not. It is used in the is_parameter_texture() function:

return uniforms[p_param].texture_order >= 0;
}

We would have to ask @reduz what his rationale was when he wrote the code

@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 23, 2023
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 14, 2023
@clayjohn clayjohn modified the milestones: 4.3, 4.x Jul 24, 2024
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants