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

Rename uniform to parameter across the engine #64952

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Aug 27, 2022

Changes:

  • ShaderMaterial::set_shader_uniform/get_shader_uniform renamed to set_shader_param/get_shader_param.
  • All other visual shader classes <Type>Uniform renamed to <Type>Parameter.
  • Other uniform methods, parameter and variables renamed to parameters in the VisualShaderEditor.
  • Separates VisualShaderNodeTextureUniform to VisualShaderNodeTextureParameter and VisualShaderNodeTexture2DParameter.
  • Changed default_value type in VisualShaderNodeVec4Parameter from Quaternion to Vector4.
  • Added compatibility classes and changed docs accordingly.

Target to close godotengine/godot-proposals#5123 hope I did everything correct (as far as I remember the last bikeshedding meeting)

@Chaosus Chaosus requested review from a team as code owners August 27, 2022 09:31
@Chaosus Chaosus added this to the 4.0 milestone Aug 27, 2022
@Chaosus Chaosus requested a review from QbieShay August 27, 2022 09:32
@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch 5 times, most recently from 3f55684 to d97985c Compare August 27, 2022 11:29
@QbieShay
Copy link
Contributor

Hey @Chaosus, thanks a lot for taking the time to do this!
I have two questions:

  1. Is the entry for "global shader unifrom" in project setting also changed?
  2. I am not sure if they preferred set_shader_param or set_shader_parameter? (@akien-mga ?)
    The rest looks good to me! (Note I didn't test, i can't this weekend)

@Chaosus
Copy link
Member Author

Chaosus commented Aug 28, 2022

Is the entry for "global shader unifrom" in project setting also changed?

In the project setting, it's called Shader Globals - how do you want to change it?

image

Also, I didn't change the name of the methods in the RenderingServer and MaterialStorage - I'm not sure that it is a good thing to do, and @reduz would certainly against it.

@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from d97985c to ee98167 Compare August 28, 2022 08:48
@clayjohn
Copy link
Member

Also, I didn't change the name of the methods in the RenderingServer and MaterialStorage - I'm not sure that it is a good thing to do, and @reduz would certainly against it.

I think anything exposed to users should use "parameter". I don't think it is a big deal to change the naming in RenderingServer and MaterialStorage. After all, reduz originally named them with parameter and it was just changed to "uniform" recently (in #59840 and #59844)

@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from ee98167 to 1b03840 Compare August 29, 2022 05:16
@Zireael07
Copy link
Contributor

I think anything exposed to users should use "parameter".

Yep, should be consistent whichever way the cookie crumbles

@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from 1b03840 to aace400 Compare August 29, 2022 07:41
@Chaosus Chaosus requested review from a team as code owners August 29, 2022 07:41
@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from aace400 to 981ba44 Compare August 29, 2022 07:42
@Chaosus Chaosus changed the title Rename uniform to parameter in visual shader classes and ShaderMaterial Rename uniform to parameter across the engine Aug 29, 2022
@Chaosus
Copy link
Member Author

Chaosus commented Aug 29, 2022

@clayjohn Alright, I've renamed uniform term to parameter across all functions and enumeration members:

image

@nathanfranke
Copy link
Contributor

nathanfranke commented Aug 29, 2022

If anyone disagrees with the naming in #64092, let me know. I decided shader_param/ for compatibility/simplicity, and since the internal properties are not user facing (users should really be using set_shader_param("foo", bar))

Edit: Changed to shader_parameter/

@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from 70739fe to 5d345da Compare August 29, 2022 14:40
@Chaosus
Copy link
Member Author

Chaosus commented Aug 29, 2022

I've added set_shader_param -> set_shader_parameter to the project converter. Also renamed bunch of parameter names and other internal functions. I leave only some internal classes untouched such as RD::Uniform, UniformSet etc - do I need to rename them too?

@QbieShay
Copy link
Contributor

In the project setting, it's called Shader Globals - how do you want to change it?

Awesome! Shader Globals works perfect.

@clayjohn
Copy link
Member

I've added set_shader_param -> set_shader_parameter to the project converter. Also renamed bunch of parameter names and other internal functions. I leave only some internal classes untouched such as RD::Uniform, UniformSet etc - do I need to rename them too?

No, RD::Uniform and Uniform set etc. should all remain as is. Just the user-facing functions to set a single uniform should be changed. I'll review now!

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. Great work! I think everything in RenderingDevice should continue to use "uniform". So you'll have to undo those changes unfortunately. I think it is best it maintains the original name as people using it will be more familiar with uniforms as uniform has a specific meaning in the context of graphics APIs that is incompatible with the more general term "parameter". Parameter is fine for the Godot-shader API though, so it is suitable for user-facing code in the editer, scene nodes and RenderingServer

drivers/vulkan/rendering_device_vulkan.h Outdated Show resolved Hide resolved
scene/resources/shader.cpp Show resolved Hide resolved
scene/resources/shader.cpp Outdated Show resolved Hide resolved
scene/resources/material.cpp Show resolved Hide resolved
servers/register_server_types.cpp Outdated Show resolved Hide resolved
servers/rendering/rendering_device.h Outdated Show resolved Hide resolved
@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from 5d345da to eeb17b4 Compare August 30, 2022 05:09
@Chaosus Chaosus requested a review from a team as a code owner August 30, 2022 05:09
@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from eeb17b4 to bdf7bff Compare August 30, 2022 05:12
@Chaosus
Copy link
Member Author

Chaosus commented Aug 30, 2022

@clayjohn Done I think - check again

@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from bdf7bff to 93c1910 Compare September 1, 2022 05:46
@reduz
Copy link
Member

reduz commented Sep 1, 2022

The actual problem is that uniform term is unfriendly to VisualShader for artists, everywhere else it should be uniform, which is the right term. "parameter" is not more friendly for programmers, its more confusing because anything can be a parameter.

Copy link
Member

@reduz reduz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also marks vulkan_core.h as changed, it should not be?

drivers/vulkan/rendering_device_vulkan.h Outdated Show resolved Hide resolved
servers/rendering/renderer_geometry_instance.h Outdated Show resolved Hide resolved
servers/rendering/renderer_geometry_instance.h Outdated Show resolved Hide resolved
servers/rendering/renderer_geometry_instance.cpp Outdated Show resolved Hide resolved
servers/rendering_server.h Outdated Show resolved Hide resolved
@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch 2 times, most recently from 7a1e7a0 to 372f97d Compare September 1, 2022 07:16
@Chaosus
Copy link
Member Author

Chaosus commented Sep 1, 2022

It also marks vulkan_core.h as changed, it should not be?

Sorry, due to formatting suddenly, reverted it back

@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from 372f97d to eb90ae9 Compare September 1, 2022 07:36
@Chaosus Chaosus requested a review from a team as a code owner September 1, 2022 07:36
@Chaosus Chaosus requested a review from reduz September 1, 2022 07:39
drivers/gles3/rasterizer_scene_gles3.cpp Outdated Show resolved Hide resolved
drivers/gles3/storage/material_storage.cpp Outdated Show resolved Hide resolved
drivers/gles3/storage/material_storage.cpp Outdated Show resolved Hide resolved
@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from eb90ae9 to 537b53d Compare September 1, 2022 07:49
@Chaosus Chaosus requested a review from reduz September 1, 2022 07:51
@Chaosus Chaosus force-pushed the vs_rename_uniform_to_param branch from 537b53d to 8191b3c Compare September 1, 2022 08:44
@akien-mga akien-mga merged commit c82bbc3 into godotengine:master Sep 2, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename API around shaders to use the word "parameter" and not "uniform" (except text shaders)
7 participants