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.

Sign up for GitHub

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

Jump to bottom

Polish rendering driver refactor further #67000

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Polish rendering driver refactor further #67000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions SConstruct
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ opts.Add(BoolVariable("deprecated", "Enable compatibility code for deprecated an
opts.Add(EnumVariable("float", "Floating-point precision", "32", ("32", "64")))
opts.Add(BoolVariable("minizip", "Enable ZIP archive support using minizip", True))
opts.Add(BoolVariable("xaudio2", "Enable the XAudio2 audio driver", False))
opts.Add(BoolVariable("vulkan", "Enable the vulkan video driver", True))
opts.Add(BoolVariable("opengl3", "Enable the OpenGL/GLES3 video driver", True))
opts.Add(BoolVariable("vulkan", "Enable the vulkan rendering driver", True))
opts.Add(BoolVariable("opengl3", "Enable the OpenGL/GLES3 rendering driver", True))
opts.Add(BoolVariable("openxr", "Enable the OpenXR driver", True))
opts.Add(BoolVariable("use_volk", "Use the volk library to load the Vulkan loader dynamically", True))
opts.Add("custom_modules", "A list of comma-separated directory paths containing custom modules to build.", "")
Expand Down
4 changes: 2 additions & 2 deletions doc/classes/ProjectSettings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2097,8 +2097,6 @@
<member name="rendering/renderer/rendering_method.web" type="String" setter="" getter="" default="&quot;gl_compatibility&quot;">
Override for [member rendering/renderer/rendering_method] on web.
</member>
<member name="rendering/rendering_device/descriptor_pools/max_descriptors_per_pool" type="int" setter="" getter="" default="64">
</member>
<member name="rendering/rendering_device/driver" type="String" setter="" getter="" default="&quot;vulkan&quot;">
Sets the driver to be used by the renderer when using a RenderingDevice-based renderer like the clustered renderer or the mobile renderer. This property can not be edited directly, instead, set the driver using the platform-specific overrides.
</member>
Expand All @@ -2123,6 +2121,8 @@
</member>
<member name="rendering/rendering_device/staging_buffer/texture_upload_region_size_px" type="int" setter="" getter="" default="64">
</member>
<member name="rendering/rendering_device/vulkan/max_descriptors_per_pool" type="int" setter="" getter="" default="64">
</member>
<member name="rendering/scaling_3d/fsr_sharpness" type="float" setter="" getter="" default="0.2">
Determines how sharp the upscaled image will be when using the FSR upscaling mode. Sharpness halves with every whole number. Values go from 0.0 (sharpest) to 2.0. Values above 2.0 won't make a visible difference.
</member>
Expand Down
2 changes: 1 addition & 1 deletion drivers/SCsub
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ SConscript("winmidi/SCsub")

# Graphics drivers
if env["vulkan"]:
SConscript("spirv-reflect/SCsub")
SConscript("vulkan/SCsub")
if env["opengl3"]:
SConscript("gl_context/SCsub")
SConscript("gles3/SCsub")

# Core dependencies
SConscript("png/SCsub")
SConscript("spirv-reflect/SCsub")

env.add_source_files(env.drivers_sources, "*.cpp")

Expand Down
8 changes: 4 additions & 4 deletions drivers/vulkan/rendering_device_vulkan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5172,9 +5172,9 @@ Vector<uint8_t> RenderingDeviceVulkan::shader_compile_binary_from_spirv(const Ve
uint32_t offset = 0;
uint8_t *binptr = ret.ptrw();
binptr[0] = 'G';
binptr[1] = 'V';
binptr[1] = 'S';
binptr[2] = 'B';
binptr[3] = 'D'; // Godot vulkan binary data.
binptr[3] = 'D'; // Godot Shader Binary Data.
clayjohn marked this conversation as resolved.
Show resolved Hide resolved
offset += 4;
encode_uint32(SHADER_BINARY_VERSION, binptr + offset);
offset += sizeof(uint32_t);
Expand Down Expand Up @@ -5235,7 +5235,7 @@ RID RenderingDeviceVulkan::shader_create_from_bytecode(const Vector<uint8_t> &p_
uint32_t read_offset = 0;
// Consistency check.
ERR_FAIL_COND_V(binsize < sizeof(uint32_t) * 3 + sizeof(RenderingDeviceVulkanShaderBinaryData), RID());
ERR_FAIL_COND_V(binptr[0] != 'G' || binptr[1] != 'V' || binptr[2] != 'B' || binptr[3] != 'D', RID());
ERR_FAIL_COND_V(binptr[0] != 'G' || binptr[1] != 'S' || binptr[2] != 'B' || binptr[3] != 'D', RID());

uint32_t bin_version = decode_uint32(binptr + 4);
ERR_FAIL_COND_V(bin_version != SHADER_BINARY_VERSION, RID());
Expand Down Expand Up @@ -9393,7 +9393,7 @@ void RenderingDeviceVulkan::initialize(VulkanContext *p_context, bool p_local_de
ERR_CONTINUE(err != OK);
}

max_descriptors_per_pool = GLOBAL_DEF("rendering/rendering_device/descriptor_pools/max_descriptors_per_pool", 64);
max_descriptors_per_pool = GLOBAL_DEF("rendering/rendering_device/vulkan/max_descriptors_per_pool", 64);

// Check to make sure DescriptorPoolKey is good.
static_assert(sizeof(uint64_t) * 3 >= UNIFORM_TYPE_MAX * sizeof(uint16_t));
Expand Down
1 change: 1 addition & 0 deletions servers/rendering/renderer_rd/shader_rd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ void ShaderRD::_build_variant_code(StringBuilder &builder, uint32_t p_variant, c
#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");
Comment on lines 180 to +183
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:

godot/modules/glslang/register_types.cpp

Lines 84 to 129 in eea14a0

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.

} break;
case StageTemplate::Chunk::TYPE_MATERIAL_UNIFORMS: {
builder.append(p_version->uniforms.get_data()); //uniforms (same for vertex and fragment)
Expand Down
5 changes: 3 additions & 2 deletions servers/rendering_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2868,12 +2868,13 @@ void RenderingServer::init() {

GLOBAL_DEF("rendering/2d/shadow_atlas/size", 2048);

// Already defined in RenderingDeviceVulkan::initialize which runs before this code.
// Already defined in some RenderingDevice*::initialize, which run before this code.
// We re-define them here just for doctool's sake. Make sure to keep default values in sync.
GLOBAL_DEF("rendering/rendering_device/staging_buffer/block_size_kb", 256);
GLOBAL_DEF("rendering/rendering_device/staging_buffer/max_size_mb", 128);
GLOBAL_DEF("rendering/rendering_device/staging_buffer/texture_upload_region_size_px", 64);
GLOBAL_DEF("rendering/rendering_device/descriptor_pools/max_descriptors_per_pool", 64);
// Vulkan-specific.
GLOBAL_DEF("rendering/rendering_device/vulkan/max_descriptors_per_pool", 64);

GLOBAL_DEF("rendering/shader_compiler/shader_cache/enabled", true);
GLOBAL_DEF("rendering/shader_compiler/shader_cache/compress", true);
Expand Down