Opened 5 years ago

Last modified 4 years ago

#3640 new enhancement

[PATCH] Shader preprocessor improvements

Reported by: Mikita Hradovich Owned by:
Priority: Nice to Have Milestone: Backlog
Component: Core engine Keywords: glsl, smaa, antialiasing, patch simple
Cc: Patch:

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)

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

Download all attachments as: .zip

Change History (18)

Changed 5 years ago by Mikita Hradovich

Initial implementation of #include directive support (IN PROGRESS)

comment:1 Changed 5 years ago by wraitii

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 Changed 5 years ago by historic_bruno

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

Changed 5 years ago by fabio

mikita patch uzipped for easier review

comment:3 Changed 5 years ago by 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.

comment:4 Changed 5 years ago by 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:))?

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

comment:5 Changed 5 years ago by 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.

comment:6 Changed 5 years ago by historic_bruno

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

Changed 5 years ago by Mikita Hradovich

Preprocessor #include support with hotloading of included files

comment:7 Changed 5 years ago by 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

comment:8 Changed 5 years ago by 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).

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.

Version 0, edited 5 years ago by wraitii (next)

comment:9 Changed 5 years ago by leper

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 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 Changed 5 years ago by Mikita Hradovich

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

comment:11 Changed 4 years ago by Palaxin

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

comment:12 Changed 4 years ago by Itms

Milestone: Alpha 20Alpha 21

comment:13 Changed 4 years ago by elexis

Keywords: simple added
Milestone: Alpha 21Backlog

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

comment:14 Changed 4 years ago by Vladislav Belov

Any progress?

Note: See TracTickets for help on using tickets.