[PATCH] Shader preprocessor improvements

[Mikita Hradovich]

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.

[Mikita Hradovich]

Initial implementation of #include directive support (IN PROGRESS)

[wraitii]

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.

[historic_bruno]

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

[fabio]

mikita patch uzipped for easier review

[wraitii]

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.

[Mikita Hradovich]

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:))

[wraitii]

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.

[historic_bruno]

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

[Mikita Hradovich]

Preprocessor #include support with hotloading of included files

[Mikita Hradovich]

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

[wraitii]

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.

[leper]

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 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

[Mikita Hradovich]

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

[elexis]

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

[Vladislav Belov]

Any progress?

[Vladislav Belov]

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