GPU: Change GLSL include directive #128076

Merged
Clément Foucault merged 16 commits from fclem/blender:glsl-include into main 2024-10-04 15:48:46 +02:00

This changes the include directive to use the standard C preprocessor
#include directive.

The regex to applied to all glsl sources is:
pragma BLENDER_REQUIRE\((\w+\.glsl)\)
include "$1"

This allow C++ linter to parse the code and allow easier codebase
traversal.

However there is a small catch. While it does work like a standard
include directive when the code is treated as C++, it doesn't when
compiled by our shader backends. In this case, we still use our
dependency concatenation approach instead of file injection.

This means that included files will always be prepended when compiled
to GLSL and a file cannot be appended more than once.

This is why all GLSL lib file should have the #pragma once directive
and always be included at the start of the file.

These requirements are actually already enforced by our code-style
in practice.

On the implementation, the source needed to be mutated to comment
the #pragma once and #include. This is needed to avoid GLSL
compiler error out as this is an extension that not all vendor
supports.

Rel #127983

TODO:

  • Fix include order hell before commiting this
  • Add #pragma once to every GLSL library file
  • Move the patching to datatoc to avoid memory duplication Moved to its own executable (see #128261)
This changes the include directive to use the standard C preprocessor `#include` directive. The regex to applied to all glsl sources is: `pragma BLENDER_REQUIRE\((\w+\.glsl)\)` `include "$1"` This allow C++ linter to parse the code and allow easier codebase traversal. However there is a small catch. While it does work like a standard include directive when the code is treated as C++, it doesn't when compiled by our shader backends. In this case, we still use our dependency concatenation approach instead of file injection. This means that included files will always be prepended when compiled to GLSL and a file cannot be appended more than once. This is why all GLSL lib file should have the `#pragma once` directive and always be included at the start of the file. These requirements are actually already enforced by our code-style in practice. On the implementation, the source needed to be mutated to comment the `#pragma once` and `#include`. This is needed to avoid GLSL compiler error out as this is an extension that not all vendor supports. Rel #127983 TODO: - [x] Fix include order hell before commiting this - [x] Add `#pragma once` to every GLSL library file - [x] ~~Move the patching to datatoc to avoid memory duplication~~ Moved to its own executable (see #128261)
Clément Foucault added the
Module
Viewport & EEVEE
label 2024-09-24 16:12:21 +02:00
Clément Foucault added 2 commits 2024-09-24 16:12:34 +02:00
Clément Foucault added 2 commits 2024-09-24 18:30:07 +02:00
Author
Member

Here is the regex I used for the renaming

#pragma BLENDER_REQUIRE\(([a-zA-Z0-9_\-\.]+)\)
#include "$1"
Here is the regex I used for the renaming ``` #pragma BLENDER_REQUIRE\(([a-zA-Z0-9_\-\.]+)\) #include "$1" ```
Clément Foucault added 1 commit 2024-09-25 19:03:04 +02:00
Clément Foucault added 1 commit 2024-09-25 19:12:09 +02:00
Clément Foucault added 1 commit 2024-09-25 21:17:04 +02:00
Clément Foucault requested review from Campbell Barton 2024-09-25 21:17:30 +02:00
Author
Member

@ideasman42 tagging you for the changes to datatoc.

@ideasman42 tagging you for the changes to datatoc.
Campbell Barton approved these changes 2024-09-26 12:56:59 +02:00
@ -174,6 +176,26 @@ int main(int argc, char **argv)
size -= size_offset; /* The comment is skipped, */
}
/* For now, the same files needs both. */

Overall, if this works, the change is small & isolated enough that any improvements can be made in main (accepting).

Having said that, here are some thoughts.

Long term I think it would work better to duplicate datatoc into a GLSL pre-processing tool, this can then depend on C++ regex library so operating on text isn't so fragile. Then more comprehensive parsing can be added without complicating datatoc.

Overall, if this works, the change is small & isolated enough that any improvements can be made in `main` (accepting). Having said that, here are some thoughts. Long term I think it would work better to duplicate `datatoc` into a GLSL pre-processing tool, this can then depend on C++ regex library so operating on text isn't so fragile. Then more comprehensive parsing can be added without complicating `datatoc`.
Author
Member

Long term I think it would work better to duplicate datatoc into a GLSL pre-processing tool, this can then depend on C++ regex library so operating on text isn't so fragile.

That would be ideal. I just finished #128172 which calls for more regex usage. I believe that will be a follow up task.

> Long term I think it would work better to duplicate datatoc into a GLSL pre-processing tool, this can then depend on C++ regex library so operating on text isn't so fragile. That would be ideal. I just finished #128172 which calls for more regex usage. I believe that will be a follow up task.
fclem marked this conversation as resolved
@ -175,2 +177,4 @@
}
/* For now, the same files needs both. */
mutate_glsl_directives_test = strip_leading_c_comments_test;

Even to get something working quickly, this could be a vector of char arrays, to allow inserting of arbitrary text.
Otherwise, if this is only for comments it could be named more clearly "cxx_comment_offsets" or similar.

Even to get something working quickly, this could be a vector of char arrays, to allow inserting of arbitrary text. Otherwise, if this is only for comments it could be named more clearly "cxx_comment_offsets" or similar.
Author
Member

Yes, I will make it a std::vector<std::pair<long, char>> so that the replaced char is explicit (can be used for other things later).

Yes, I will make it a `std::vector<std::pair<long, char>>` so that the replaced char is explicit (can be used for other things later).
fclem marked this conversation as resolved
@ -177,0 +179,4 @@
/* For now, the same files needs both. */
mutate_glsl_directives_test = strip_leading_c_comments_test;
std::vector<long> mutations;

A short description of what's being done here would be good (commenting our per-processing lines).

Worth mentioning the strange resulting //nclude literal which Blender knows to check for.

A short description of what's being done here would be good (commenting our per-processing lines). Worth mentioning the strange resulting `//nclude` literal which Blender knows to check for.
fclem marked this conversation as resolved
@ -177,0 +184,4 @@
char buffer[1024];
long original_pos = ftell(fpin);
long pos = original_pos;
while (fgets(buffer, 1024, fpin)) {

picky 1024 -> sizeof(buffer).

picky `1024` -> `sizeof(buffer)`.
fclem marked this conversation as resolved
Clément Foucault added 2 commits 2024-09-26 15:31:57 +02:00
Clément Foucault added 1 commit 2024-09-26 15:38:31 +02:00
Clément Foucault changed title from WIP: GPU: Change GLSL include directive to GPU: Change GLSL include directive 2024-09-26 16:06:14 +02:00
Clément Foucault added 1 commit 2024-09-26 16:06:17 +02:00
Add pragma once to all glsl libs
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
271c76024c
Author
Member

@blender-bot build

@blender-bot build
Clément Foucault requested review from Jeroen Bakker 2024-09-26 16:07:53 +02:00
Clément Foucault requested review from Miguel Pozo 2024-09-26 16:08:00 +02:00
Ray molenkamp requested changes 2024-09-26 16:12:53 +02:00
Ray molenkamp left a comment
Member

I'm gonna disagree here, if transformations need to be done, datatoc is not the place to do them, yes it's convenient but it just doesn't belong there.

I'm gonna disagree here, if transformations need to be done, datatoc is not the place to do them, yes it's convenient but it just doesn't belong there.
Member

Since we are at it, can't we just use something like shaderc PreprocessGlsl and have a real preprocessor?

Since we are at it, can't we just use something like `shaderc` `PreprocessGlsl` and have a real preprocessor?
Author
Member

@LazyDodo Then where? Do we want a copy of datatoc right now for so little? I am fine doing it in a follow up task to not make this one grow in scope.

@pragma37 What do you mean by that? If you mean that all #include should be done by shaderc then that has way more potential for breaking the current pipeline. And we need to do these includes at runtime (because of material shaders) at this point we don't have any filesystem structure. Maybe it is possible but that sounds orthogonal to this task.

@LazyDodo Then where? Do we want a copy of `datatoc` right now for so little? I am fine doing it in a follow up task to not make this one grow in scope. @pragma37 What do you mean by that? If you mean that all `#include` should be done by `shaderc` then that has way more potential for breaking the current pipeline. And we need to do these includes at runtime (because of material shaders) at this point we don't have any filesystem structure. Maybe it is possible but that sounds orthogonal to this task.
Member

datatoc's job is to take a arbitrary file and turn it into some c code for embedding, nothing more, nothing less, any additional processing doesn't belong in it, writing a nearly identical datatoc also isn't the way to go, why would a tool that performs whatever transformation you desire also perform datatoc's tasks? You can chain multiple tools together in CMake they all can just do the job they excel at and pass it on to the next one.

datatoc's job is to take a arbitrary file and turn it into some c code for embedding, nothing more, nothing less, any additional processing doesn't belong in it, writing a nearly identical datatoc also isn't the way to go, why would a tool that performs whatever transformation you desire _also_ perform datatoc's tasks? You can chain multiple tools together in CMake they all can just do the job they excel at and pass it on to the next one.
Author
Member

@LazyDodo I do agree, but can't that be split to a follow up task as this will grow in complexity (have to deal with cmake and that might need your involvement)?

@LazyDodo I do agree, but can't that be split to a follow up task as this will grow in complexity (have to deal with cmake and that might need your involvement)?
Member

Please let me do the bad thing, otherwise i'll have to ask you for help, isn't the argument you think it may be, as I'll be happy to sink in whatever time is needed to prevent the bad thing from happening.

How can i help? :)

Please let me do the bad thing, otherwise i'll have to ask you for help, isn't the argument you think it may be, as I'll be happy to sink in whatever time is needed to prevent the bad thing from happening. How can i help? :)
Member

And we need to do these includes at runtime (because of material shaders) at this point we don't have any filesystem structure.

I only took a quick look, but PreprocessGlsl takes a CompileOptions parameter where you can add callbacks to handle include directives (IncluderInterface).

> And we need to do these includes at runtime (because of material shaders) at this point we don't have any filesystem structure. I only took a quick look, but `PreprocessGlsl` takes a `CompileOptions` parameter where you can add callbacks to handle include directives (`IncluderInterface`).
Author
Member

I only took a quick look, but PreprocessGlsl takes a CompileOptions parameter where you can add callbacks to handle include directives (IncluderInterface).

Looks like it! Might be worth looking into. Hopefully that doesn't mangle our error log system.

> I only took a quick look, but PreprocessGlsl takes a CompileOptions parameter where you can add callbacks to handle include directives (IncluderInterface). Looks like it! Might be worth looking into. Hopefully that doesn't mangle our error log system.
Member

I do have to agree about datatoc. however it seems that currently we already do stripping in datatoc which should also be moved to a separate tool.

Personally I don't see the issue of adding the right tool in a separate and land it before this one as the impact is limited. If you need help with that I can put it on my list as well.

I do have to agree about datatoc. however it seems that currently we already do stripping in datatoc which should also be moved to a separate tool. Personally I don't see the issue of adding the right tool in a separate and land it before this one as the impact is limited. If you need help with that I can put it on my list as well.
Clément Foucault added 4 commits 2024-10-04 14:33:10 +02:00
# Conflicts:
#	source/blender/datatoc/datatoc.cc
#	source/blender/draw/engines/eevee_next/shaders/eevee_light_eval_lib.glsl
# Conflicts:
#	source/blender/draw/intern/shaders/common_intersect_lib.glsl
Remove changes to datatoc
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
9732759833
Author
Member

@blender-bot build

@blender-bot build
Clément Foucault added 1 commit 2024-10-04 14:42:13 +02:00
Clément Foucault merged commit 7e5bc58649 into main 2024-10-04 15:48:46 +02:00
Clément Foucault deleted branch glsl-include 2024-10-04 15:48:50 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#128076
No description provided.