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

Only build glslang if Vulkan or Direct3D 12 rendering is enabled #84613

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Nov 8, 2023

glslang isn't needed for OpenGL rendering, which includes the web export. This reduces the web release export template's .wasm size by about 20 KB, since web builds use vulkan=no.

This check may need to be revisited once #70315 is merged, as I assume the Direct3D 12 renderer also needs the glslang module to be enabled.

I found this while working on godotengine/godot-docs#8424 🙂

@Calinou Calinou requested a review from a team as a code owner November 8, 2023 11:44
@Calinou Calinou added this to the 4.x milestone Nov 8, 2023
@Calinou Calinou force-pushed the glslang-build-only-vulkan branch from 146f13a to 7a6fcff Compare November 8, 2023 11:45
@clayjohn
Copy link
Member

clayjohn commented Nov 8, 2023

Shouldn't this be tied into building for web instead? So it is disabled on web builds and enabled on non-web builds?

@Calinou
Copy link
Member Author

Calinou commented Nov 8, 2023

Shouldn't this be tied into building for web instead? So it is disabled on web builds and enabled on non-web builds?

If you build a custom desktop export template with all Vulkan-based renderers disabled, you don't need glslang either as I understand it. (vulkan=no can be used for desktop builds too.)

@akien-mga
Copy link
Member

@RandomShaper Do we need glslang for D3D 12?

@RandomShaper
Copy link
Member

@RandomShaper Do we need glslang for D3D 12?

We need. For now (including upcoming refactors), glslang is needed for every RenderingDevice backend. In an upcoming PR I'm letting the RD drivers build scripts define RD_ENABLED. That would be the hint to include glslang or not.

Going back to the present, this PR is fine, but will make me add or env['d3d12'] to the script in the D3D12 PR. That's fine as I'll have to rebase anyway and reapply to D3D12 everything happened to Vulkan. However, for future-proofing it, you may want to add it yourself.

glslang isn't needed for OpenGL rendering, which includes the web export.
This reduces the web release export template's `.wasm` size by about 20 KB,
since web builds use `vulkan=no`.
@Calinou Calinou force-pushed the glslang-build-only-vulkan branch from 7a6fcff to 2aaa4cd Compare January 10, 2024 23:43
@Calinou
Copy link
Member Author

Calinou commented Jan 10, 2024

Going back to the present, this PR is fine, but will make me add or env['d3d12'] to the script in the D3D12 PR. That's fine as I'll have to rebase anyway and reapply to D3D12 everything happened to Vulkan. However, for future-proofing it, you may want to add it yourself.

I've done this in this PR, since D3D12 has been merged in master.

@Calinou Calinou changed the title Only build glslang if Vulkan rendering is enabled Only build glslang if Vulkan or Direct3D 12 rendering is enabled Jan 10, 2024
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jan 11, 2024
@akien-mga akien-mga merged commit 5cdd0c6 into godotengine:master Jan 11, 2024
15 checks passed
@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.

4 participants