Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#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


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) (3.3 KB ) - added by Mikita Hradovich 9 years ago.
Initial implementation of #include directive support (IN PROGRESS)
preprocessor_include_initial_impl.patch (11.0 KB ) - added by fabio 9 years ago.
mikita patch uzipped for easier review
preprocessor_include_with_hotload_impl.patch (16.9 KB ) - added by Mikita Hradovich 9 years ago.
Preprocessor #include support with hotloading of included files
preprocessor_include_with_hotload_impl_fixed.patch (9.8 KB ) - added by wraitii 9 years ago.

Download all attachments as: .zip

Change History (22)

by Mikita Hradovich, 9 years ago

Initial implementation of #include directive support (IN PROGRESS)

comment:1 by wraitii, 9 years ago

Keywords: patch review added
Milestone: BacklogAlpha 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 historic_bruno, 9 years ago

No need to zip the patch, just attach it directly so it can be previewed in Trac :)

by fabio, 9 years ago

mikita patch uzipped for easier review

comment:3 by wraitii, 9 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 Mikita Hradovich, 9 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:))?

Last edited 9 years ago by Mikita Hradovich (previous) (diff)

comment:5 by wraitii, 9 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 historic_bruno, 9 years ago

Hotloading is extremely useful for shader development (in my experience), so I agree that would be good to have :)

by Mikita Hradovich, 9 years ago

Preprocessor #include support with hotloading of included files

comment:7 by Mikita Hradovich, 9 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

comment:8 by wraitii, 9 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.

Last edited 9 years ago by wraitii (previous) (diff)

comment:9 by leper, 9 years ago

Keywords: review removed

Some comments originally posted on irc (

  • Should use the same hotloading code as all other places that do that
  • & in that for loop
  • if (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 one
  • ENSURE(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:10 by Mikita Hradovich, 9 years ago

Thanks for review, I'm working on fixing of problems you've pointed out

comment:11 by Palaxin, 8 years ago

Summary: [Patch] Shader preprocessor improvements[PATCH] Shader preprocessor improvements

comment:12 by Itms, 8 years ago

Milestone: Alpha 20Alpha 21

comment:13 by elexis, 8 years ago

Keywords: simple added
Milestone: Alpha 21Backlog

Tagging as simple as apparently only few things of the existing patch need to be fixed.

comment:14 by Vladislav Belov, 8 years ago

Any progress?

comment:15 by Stan, 4 years ago

Milestone: BacklogAlpha 24
Patch: Phab:D3030

comment:16 by Vladislav Belov, 4 years ago

Owner: set to Vladislav Belov
Status: newassigned

comment:17 by Vladislav Belov, 4 years ago

Resolution: fixed
Status: assignedclosed

comment:18 by Vladislav Belov, 4 years ago

#elif directives and others are implemented by new version of OgrePreprocessor that was added in r23404.

Note: See TracTickets for help on using tickets.