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

Polish rendering driver refactor further #67000

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Oct 6, 2022

(A little extension to #65541.)

Mainly:

  • Make max_descriptors_per_pool project setting Vulkan-specific.
  • Use a common, render driver agnostic magic FourCC for shader binary data.
  • Downgrade spirv_reflect to Vulkan-only dependency.
  • Add a RENDER_DRIVER_* macro to GLSL shader code for per-driver customizations.

Mainly:
- Make `max_descriptors_per_pool` project setting Vulkan-specific.
- Use a common, render driver agnostic magic FourCC for shader binary data.
- Downgrade spirv_reflect to Vulkan-only dependency.
- Add a `RENDER_DRIVER_*` macro to GLSL shader code for per-driver customizations.
Comment on lines 180 to +183
#if defined(MACOS_ENABLED) || defined(IOS_ENABLED)
builder.append("#define MOLTENVK_USED\n");
#endif
builder.append(String("#define RENDER_DRIVER_") + OS::get_singleton()->get_current_rendering_driver_name().to_upper() + "\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding the RENDER_DRIVER_* defines here means that shaders that are created via RenderingDevice::shader_compile_spirv_from_source alone won't have access to them.

Maybe the defines should be added here:

std::string preamble = "";
shader.setStrings(&cs_strings, 1);
shader.setEnvInput(glslang::EShSourceGlsl, stages[p_stage], glslang::EShClientVulkan, ClientInputSemanticsVersion);
shader.setEnvClient(glslang::EShClientVulkan, ClientVersion);
shader.setEnvTarget(glslang::EShTargetSpv, TargetVersion);
if (check_subgroup_support) {
uint32_t stage_bit = 1 << p_stage;
uint32_t subgroup_in_shaders = uint32_t(p_render_device->limit_get(RD::LIMIT_SUBGROUP_IN_SHADERS));
uint32_t subgroup_operations = uint32_t(p_render_device->limit_get(RD::LIMIT_SUBGROUP_OPERATIONS));
if ((subgroup_in_shaders & stage_bit) == stage_bit) {
// stage supports subgroups
preamble += "#define has_GL_KHR_shader_subgroup_basic 1\n";
if (subgroup_operations & RenderingDevice::SUBGROUP_VOTE_BIT) {
preamble += "#define has_GL_KHR_shader_subgroup_vote 1\n";
}
if (subgroup_operations & RenderingDevice::SUBGROUP_ARITHMETIC_BIT) {
preamble += "#define has_GL_KHR_shader_subgroup_arithmetic 1\n";
}
if (subgroup_operations & RenderingDevice::SUBGROUP_BALLOT_BIT) {
preamble += "#define has_GL_KHR_shader_subgroup_ballot 1\n";
}
if (subgroup_operations & RenderingDevice::SUBGROUP_SHUFFLE_BIT) {
preamble += "#define has_GL_KHR_shader_subgroup_shuffle 1\n";
}
if (subgroup_operations & RenderingDevice::SUBGROUP_SHUFFLE_RELATIVE_BIT) {
preamble += "#define has_GL_KHR_shader_subgroup_shuffle_relative 1\n";
}
if (subgroup_operations & RenderingDevice::SUBGROUP_CLUSTERED_BIT) {
preamble += "#define has_GL_KHR_shader_subgroup_clustered 1\n";
}
if (subgroup_operations & RenderingDevice::SUBGROUP_QUAD_BIT) {
preamble += "#define has_GL_KHR_shader_subgroup_quad 1\n";
}
}
}
if (p_render_device->has_feature(RD::SUPPORTS_MULTIVIEW)) {
preamble += "#define has_VK_KHR_multiview 1\n";
}
if (!preamble.empty()) {
shader.setPreamble(preamble.c_str());
}

Copy link
Member Author

Choose a reason for hiding this comment

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

They do work there. The D3D12 PR (#64304) already does the same and it works for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm referring to shaders that do not use ShaderRD. For example this shader fails to compile:

var rd = RenderingServer.get_rendering_device()
var source = RDShaderSource.new()
source.language = RenderingDevice.SHADER_LANGUAGE_GLSL
source.source_vertex = """
	#version 460

	#ifdef RENDER_DRIVER_VULKAN 
		void main()
		{

		}
	#endif	
"""
var shader_spirv = rd.shader_compile_spirv_from_source(source, false)
print(shader_spirv.compile_error_vertex)

ERROR: Linking vertex stage: Missing entry point: Each stage requires one entry point

Copy link
Member Author

Choose a reason for hiding this comment

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

I see.

RENDER_DRIVER_* was meant for internal engine use, but it may be worth allowing such an usage. However, in that case, I'd also do the same change to MOLTENVK_USED and any other platform or driver dependant macro. My take would be to keep this PR as it is and address the whole macro exposure change in a separate PR.

I'd like to know @clayjohn's thoughts on this.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, let's re-assess this in a follow-up issue/PR.

@akien-mga akien-mga merged commit 3306ffe into godotengine:master Oct 11, 2022
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the split_render_further branch October 11, 2022 08:15
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.

4 participants