GPU: Change GLSL include directive #128076
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#128076
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "fclem/blender:glsl-include"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
directiveand 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 GLSLcompiler error out as this is an extension that not all vendor
supports.
Rel #127983
TODO:
#pragma once
to every GLSL library fileMove the patching to datatoc to avoid memory duplicationMoved to its own executable (see #128261)Here is the regex I used for the renaming
@ideasman42 tagging you for the changes to datatoc.
@ -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 complicatingdatatoc
.That would be ideal. I just finished #128172 which calls for more regex usage. I believe that will be a follow up task.
@ -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.
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).@ -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.@ -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)
.gpu_glsl_cpp_stubs
as a GLSL source. Bypass include. fc5c1cd10eWIP: GPU: Change GLSL include directiveto GPU: Change GLSL include directive@blender-bot build
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.
Since we are at it, can't we just use something like
shaderc
PreprocessGlsl
and have a real preprocessor?@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 byshaderc
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.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.
@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)?
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? :)
I only took a quick look, but
PreprocessGlsl
takes aCompileOptions
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 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.
@blender-bot build