#3640 closed enhancement (fixed)
[PATCH] Shader preprocessor improvements
Reported by: | Mikita Hradovich | Owned by: | Vladislav Belov |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 24 |
Component: | Core engine | Keywords: | glsl, smaa, antialiasing, patch simple |
Cc: | Patch: | Phab:D3030 |
Description
Current implementation of GLSL shader preprocessor do not support some directives (e.g. #include, #define <expression>, , #elif). This directives are important for, in instance, SMAA adaptation.
Attachments (4)
Change History (22)
by , 8 years ago
Attachment: | preprocessor_include_initial_impl.zip added |
---|
comment:1 by , 8 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 20 |
Summary: | Shader preprocessor improvements → [Patch] Shader preprocessor improvements |
Thanks for the patch. We'll try to review it as quickly as possible (though I should warn you that sometimes reviewing is slow. Feel free to nag on #0ad-dev or produce other patches in the meantime.
comment:2 by , 8 years ago
No need to zip the patch, just attach it directly so it can be previewed in Trac :)
by , 8 years ago
Attachment: | preprocessor_include_initial_impl.patch added |
---|
mikita patch uzipped for easier review
comment:3 by , 8 years ago
Hey mikita, I've checked your patch and it seems to work well. I take it the shaders in the patch were just there to demonstrate that it worked?
Some comments on form: we use tabs for indenting and spaces to align elements. Your patch also had a few whitespace changes, try to remove those before putting a patch up for review as it becomes harder to see what changed.
I'll commit the C++ changes once you have answered my question.
comment:4 by , 8 years ago
Thanks for review. I'll fix whitespaces. Sorry for this. I've thought spaces are used instead of tabs
Also this functionality isn't complete in a sense that changes in included files are not tracked, only in shader files itself. Should I do this to complete this task (I think so:))?
comment:5 by , 8 years ago
For hotloading you mean? If so I would say yes, it would be good.
BTW it seems you are right that this particular file uses spaces, not tabs, so it should carry on using spaces, sorry about the confusion. Most files use tabs for alignement though.
comment:6 by , 8 years ago
Hotloading is extremely useful for shader development (in my experience), so I agree that would be good to have :)
by , 8 years ago
Attachment: | preprocessor_include_with_hotload_impl.patch added |
---|
Preprocessor #include support with hotloading of included files
comment:7 by , 8 years ago
I've just attached improved version of #include directive support. If included file changed, the whole program is reloaded.
To prevent tight coupling between Shader Manager and Preprocessor, callback is passed all the way down to Preprocessor. Preprocessor calls this callback once included file is encountered. This callback associates current program and this file allowing to reload shader program if file changes
After a few minutes of testing everything works fine. The only problem I find is that after a chain of quick changes reloaded file can be empty. It loads without errors but it's empty. I've added assert just in case.
Another *potencial* problem is that if shader no longer depends on some included file, this connection is not reset (this is not real problem, changes in file will only cause unnecessary program reloading)
There are TODOs to also add hotload support for XML files, a think this can be done associating files with effect instead of shader program and reloading whole effect. This also allow us to reset all dependencies between files and solve problem described just above
by , 8 years ago
Attachment: | preprocessor_include_with_hotload_impl_fixed.patch added |
---|
comment:8 by , 8 years ago
Attached the same patch as above, but removed the whitespace changes (technically those followed the file style, but changes for the sake of changes is not something we do usually).
Fixed (somewhat bluntly) a potential non-null-terminated string issue.
Removed "auto" and used a range-based for loop. Unsure about the use of std::function, if there is a more elegant solution I'll take it.
Otherwise I think this can be committed, works fine.
comment:9 by , 8 years ago
Keywords: | review removed |
---|
Some comments originally posted on irc (http://irclogs.wildfiregames.com/2016-01-02-QuakeNet-%230ad-dev.log)
- Should use the same hotloading code as all other places that do that
&
in that for loopif (program)
, not!= NULL
, what else should it be?- parameter names in preprocessor.h
- Ss there any gurantee about the length and whether
this->Body.String
is\0
-terminated?- if not, fix that strncpy
- if yes, still fix it
- Do we leak
buffer
in that copy function? std::string filename = std::string(t.String).substr(0, t.Length);
there is a better ctor you could use there, and that would also not need another call- That
"
check seems a bit strange - Checking for failure to find the included file, then reporting that and continuing anyway?
const CStr& input = includedFile.GetAsString();
Why&
, you don't get oneENSURE(input.size() > 0);
->!.empty()
- We also leak
result
, don't we? - Use spaces to align the condition for that while if you have to
comment:11 by , 8 years ago
Summary: | [Patch] Shader preprocessor improvements → [PATCH] Shader preprocessor improvements |
---|
comment:12 by , 8 years ago
Milestone: | Alpha 20 → Alpha 21 |
---|
comment:13 by , 8 years ago
Keywords: | simple added |
---|---|
Milestone: | Alpha 21 → Backlog |
Tagging as simple as apparently only few things of the existing patch need to be fixed.
comment:15 by , 3 years ago
Milestone: | Backlog → Alpha 24 |
---|---|
Patch: | → Phab:D3030 |
comment:16 by , 3 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:17 by , 3 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:18 by , 3 years ago
#elif directives and others are implemented by new version of OgrePreprocessor that was added in r23404.
Initial implementation of #include directive support (IN PROGRESS)