Decouple shader programs from the C++ backend infra

[bigbike]

Motivation and Context

This PR is to separate the shader programs, which is currently hard coded, from the C++ infra. Individual shader programs are now put in the folder called "shaders". In the future, user can code it up her own shaders and apply them in our simulator.
Benefit:
-) This is the 1st step to fully support rendering Replica model with advanced features, such as PTex textures, mirror reflections, HDR etc.
-) Provide more rendering flexibility. User can customize the shader based on their needs without recompiling the simulator;

How Has This Been Tested

Tested it on mac so far. I will test on Linux as well.
Tested the rendering in the viewer (utility) with MP3D and Replica model (vertex colored). Everything is OK.

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have completed my CLA (see CONTRIBUTING)
• I have added tests to cover my changes.
• All new and existing tests passed.

[erikwijmans]

For this to work with setup.py, let's move the shaders to habitat_sim/shaders, then add include_package_data=True to the setup(...) call in setup.py, and finally, there needs to be a MANIFEST.in file with graft habitat_sim/shaders in it.

EDIT: Nevermind, hadn't full thought this out -- the path of the shaders needed to load them would be hard to figure out from c++.

[erikwijmans]

Or, maybe we can compile the shader source files into the binary itself?

[erikwijmans]

Actually, doing the setup.py thing is probably best as it keeps the spirit of the PR. The path of the shader directory will just need to be set-able at runtime. The shader directory is then just os.path.join(os.path.dirname(habitat_sim.__file__), "shaders")) :)

[bigbike]

Actually, doing the setup.py thing is probably best as it keeps the spirit of the PR. The path of the shader directory will just need to be set-able at runtime. The shader directory is then just os.path.join(os.path.dirname(habitat_sim.__file__), "shaders")) :)

In my current implementation, the system tries to find the shaders by the shaderPath + shaderFilename.
"shaderPath" is fixed and set in the CMakeList.txt. It is actually the full path to the folder "shaders" under the "src". That means a user has to copy her shader programs to this folder in order to use them;

By the current design, "shaderFilename" can be provided during run-time by the user. After all shaders (e.g., vert, tcs, tes, geom, frag) are provided, user calls the function "loadShaders()" to equip the simulator with them;

[erikwijmans]

Yes, but if the user deletes /path/to/habitat_sim after installing, the shaders will no longer exist.

[msavva]

Thanks for opening this PR @bigbike -- this has long been a pain point, and it would be great to decouple shader source from C++ code as we flesh out our shading more fully.

A high-level comment (before reviewing the code in any depth). I think we can achieve decoupling of shaders from other source code in a more lightweight and flexible way by leveraging the file resource management functionality that Magnum+Corrade already provides for us 🙂You can see how this is used in the the textured-triangle example and relevant documentation is here. I'm sure @mosra could offer even more pointers and advice regarding this.

[mosra]

I have to admit I'm not a big fan of the GenericShader abstraction, especially the loadShaders() 😅 From my experience, different shaders rarely share a common setup code (at the very least there are very diverse sets of uniforms / texture binding functions) and I'm afraid having that all in a single function won't make adding new features any easier -- you'd end up copypasting and adapting the setup code anyway. This would also make it harder to use Magnum's builtin shaders along with these, as the builtin ones aren't derived from GenericShader.

The main goal of this PR is to make shaders tweakable without recompiling the C++ code, right? I think that doesn't necessarily mean we need to store them as separate files loaded at runtime -- because that means you need to hardcode or somehow detect the path to them, unnecessarily complicating things. As @msavva pointed out, I'd suggest using Utility::Resource and compile the shader sources into the final executable. With that you get:

• Ability to edit the shader source in a dedicated file, so you get proper syntax highlighting etc.
• But shaders will be still a part of the binary, thus impossible to get lost
• And you don't lose the ability to edit them at runtime either, it'd only be an opt-in functionality, because (a wild guess) 90% of the time you don't need that. It's the overrideGroup() function, where you point it to a resource file on the filesystem, overriding the compiled-in one. This opt-in functionality could be requested from command-line arguments, an environment variable etc.

By the way (we discussed this with @mathfac at CVPR): there's the Utility::Tweakable class that allows you to live-edit constants in C++ code with realtime feedback in the app. Right now it supports only numeric constants (color and angle literals as well), but could be extended for strings as well, if there's interest. That could be an alternative solution to avoid recompiling while iterating on shaders or other stuff that needs a lot of fiddling with.

[bigbike]

@msavva, @mosra : "Use Utility::Resource". I read the "textured triangle" code before coding. My concern is that an extra config file (specifying shader filenames) would be introduced to the system.

@mosra:
The purpose of this PR:
-) Get rid of the shader code directly in a string since it is hard to edit, debug and maintain.
-) We need to support Replica model (aka FRL model) in the simulator, and putting the shaders separately is the 1st step. After we do it, we can leverage the shaders shipped in the ReplicaSDK (found here: https://github.com/facebookresearch/Replica-Dataset). And I do not want to copy+paste them as long strings into our C++ source.
(btw, currently our system can only render the vertex-colored FRL model, not the ones with their customized PTex textures.)
-) "making shaders tweakable without recompiling the C++ code". This is the ideal case and was indeed in my initial purposes. But I gave up after I dig into the "GenericShader" and "PTexMeshShader". The reason is the same as what you mentioned in his comment. They are highly customized classes that work exclusively with the shipped shaders (the direct strings at the beginning of the code). If such shaders are replaced by user's own ones, the system would fail.

When you mentioned "you are not a big fan of GenericShader", are you actually talking about the "BaseShader" introduced in this PR? If yes, I have the same feelings. I already explained my initial purpose above, and found it was not practical afterwards. The code is just heavily shader-specific. So I tried to make a compromise here, only storing the {vert, geom, frag} shaders' filenames, and defining a pure virtual function loadShaders, which required developer to implement (based on her shaders).

In terms of "GenericShader": it exists in the code since the very beginning. And I agree with the naming is confusing. It just refers to the shaders that can render any meshes other than "PTexMesh", according to my understanding.

In terms of "Utility::Tweakable": When it is fully implemented, user can modify the shader source code without re-compiling the system (assuming shaders are already compiled into binary)? It is fancy. But I doubt if we can used it due to our urgent needs.

In terms of "configure_file": It seems it can avoid the config file. Do you think it is still a good idea after reading my comments?

@ALL:
So my needs and thoughts are as follows:
-) Use plain text file to store the shader code, not the direct strings.
-) I am not against pre-compiling the shaders into binary.
-) I need to choose GL version based on the OS system. Shaders in ReplicaSDK requires GL430. This is not a problem for the Linux, but a problem for Mac OS.
-) It would be great to avoid extra config files. But I am still open to this idea. I just worried about the system will end up with a number of config files, which is a burden to the users (We already have two configs coming on the way from the "physics" part). It would be convenient that user can specify the "shaders" in the python code. (But again, user's own shader without customized C++ class would not work.)
-) To fully support the rendering of the Replica models, I may need to further customize the current "PTexMeshShader" class in order to make the replicaSDK shaders to work (future work, NOT in this PR).

Let me know your thoughts towards the final solutions. Thanks!

[erikwijmans]

It would be great to avoid extra config files. But I am still open to this idea. I just worried about the system will end up with a number of config files, which is a burden to the users (We already have two configs coming on the way from the "physics" part). It would be convenient that user can specify the "shaders" in the python code. (But again, user's own shader without customized C++ class would not work.)

The configure file suggested by @mosra is not something users would need to worry about. It is just a nicer way to do STR(SHADER_DIR). On the cmake setup of the build, cmake can generate a configure.h file that just contains things like static const std::string SHADER_DIR = "/path/to/shader";.

[bigbike]

@erikwijmans : The method, which does require the extra config file can be found here: https://doc.magnum.graphics/magnum/examples-textured-triangle.html

It is totally different from the "configure.h" method that Vladimir mentioned in this PR. (I like the "configure.h" idea by the way.)

[erikwijmans]

Yeah, that configure.h thing is different than using Utility::Resource and I now see that you were referring to that :).

WRT to Utility::Resource, AFAIK, the user would only need to touch a config file if they wanted to add additional shaders. I do like the idea of baking shaders into the binary as that deals with the install issue in a more robust way.

[msavva]

I actually think the right way to go is to remove the BaseShader abstraction and eventually remove the GenericShader as well, instead relying on Magnum to provide the "basic shader". The presence of these two abstractions is really not buying us much -- we might as well not wrap Magnum shaders if there is no clear reason to do so. Also, it will be easier to leverage Magnum's suite of default shaders (e.g., for having an out-of-the-box Phong shader with which we can better shade assets). We discussed this direction with @mosra a while back, and the only missing piece on the Magnum side is the "object id" shader functionality.

For the more specific points:

@ALL:
So my needs and thoughts are as follows:
-) Use plain text file to store the shader code, not the direct strings.

Definitely 👍-- this is the most important improvement in my mind, and can be cleanly achieved using the Utility::Resource workflow.

-) I am not against pre-compiling the shaders into binary.

👍I think this is the way to go and will avoid all sorts of "path-to-the-shader-file" headaches down the line.

-) I need to choose GL version based on the OS system. Shaders in ReplicaSDK requires GL430. This is not a problem for the Linux, but a problem for Mac OS.

There are convenient functions for querying GL version and extension support in Magnum that can be used for this (see example with conditional GL version handling in the WebGL branch)

-) It would be great to avoid extra config files. But I am still open to this idea. I just worried about the system will end up with a number of config files, which is a burden to the users (We already have two configs coming on the way from the "physics" part). It would be convenient that user can specify the "shaders" in the python code. (But again, user's own shader without customized C++ class would not work.)

For the purpose of cleaning up our current shader code, I don't think the addition of a Utility::Resource conf file listing our shaders is problematic. In any case, users don't have to care about this file as they are always free to define and use shaders beyond the ones we package.

-) To fully support the rendering of the Replica models, I may need to further customize the current "PTexMeshShader" class in order to make the replicaSDK shaders to work (future work, NOT in this PR).

Yes, updates to PTexMeshShader and PTexMeshDrawable are likely needed to make sure we conform to the latest version of the ReplicaSDK shaders. Agree that it should not be part of this PR which should aim to just cleanly decouple shader code from C++ code.

[mosra]

They are highly customized classes that work exclusively with the shipped shaders (the direct strings at the beginning of the code). If such shaders are replaced by user's own ones, the system would fail.

Yes, that's what I'm saying -- the shader code and its application interface (whether C++ or Python) are highly coupled and making a generic interface that can accept any shader code is practically impossible.

Suppose you improve a builtin shader to access lightprobe data in order to figure out dynamic lighting. That means you need to supply it some uniform or bind a texture for every object it renders -- which means you need to add an C++/Python API for that, it's never about just supplying a different GLSL source.

The only way out of this I can see is, as @msavva says, removing GenericShader / BaseShader and giving the user a full control over shader interface -- so they can fully define it in Python from ground up and provide all uniform setters and texture/buffer binding points, like they would do in C++. I'm working on exposing all the shader APIs in Python.

Utility::Resource and the extra config file

This is purely a build-time thing -- the resources.conf file is there mainly because expressing (and parsing) the same in CMake would be a nightmare, additionally it allows the aforementioned runtime resource reload in the rare case you need it. By no means the users are forced to fiddle with random config files, they have the full freedom to have a shader source hardcoded in a string, loaded from a file or generated on-the-fly.

I need to choose GL version based on the OS system. Shaders in ReplicaSDK requires GL430. This is not a problem for the Linux, but a problem for Mac OS.

Looking at the sources, the only thing that requires GLSL 4.30 is the SSBO in atlas.glsl (which could be replaced with GL::BufferTexture, as is already done here?). So I'd say choosing GL 4.1/4.2 on both would make the code simpler?

[bigbike]

hmm...
Rebased on or merged from master branch, and it shows in my branch as change.
Strange.
How can I get rid of it?

[erikwijmans]

Did you merge with master or did you just cherry-pick the commit of #118 (the commit history makes it seems like the latter was done).

[bigbike]

@erikwijmans : No, I did not do cherry-pick. I did rebase or merge master though. Very strange.

[mosra]

This can be easily fixed with a manual merge (squash everything into a single commit and then remove this change). I can do that once we all approve :)

[bigbike]

@mosra :
Just updated a version using the Utility::Resource.
It took me a while to figure out that I need to add CORRADE_RESOURCE_INITIALIZE(ShaderResources); // ShaderResources is defined in the CMakeList.txt in the gfx module;
to the utility "viewer" to make it work.

I have not tried the headless mode with python. I suspect in order to make the python simulator work, I may also need to apply it somewhere else (e.g., Simulator.cpp in the gfx module), shall I?

[mosra]

Thank you!

[bigbike]

@mosra :
-) Thank you for the advices on importing the resources. I fixed the code accordingly.
-) I disabled the CORRADE_USE_PEDANTIC_FLAGS in the viewer, and the gfx lib, but it still gave me a lot of linking warnings, such as:

ld: warning: direct access in function 'Assimp::IOSystem::CurrentDirectory() const' from file 'deps/magnum-plugins/src/MagnumPlugins/AssimpImporter/libAssimpImporter.a(AssimpImport er.cpp.o)' to global weak symbol 'Assimp::IOSystem::CurrentDirectory() const::Dummy' from file 'deps/assimp/code/libassimp.a(Importer.cpp.o)' means the weak symbol cannot be overri dden at runtime. This was likely caused by different translation units being compiled with different visibility settings.

I do not think it is related to the CORRADE_USE_PEDANTIC_FLAGS as I also saw similar ones from the "datatools".
So not so sure if we are talking about the same kind of warnings.

@ALL:
-) the code is ready for review.
-) I do not know how the changes in the .circleCI folder come into this PR. If you are not happy with it, I can create another clean PR after the review is done.

[mosra]

Looks great to me! For the CircleCI change, I can do a manual merge and revert that while merging.

Added the CORRADE_USE_PEDANTIC_FLAGS to the list in #42, together with the Assimp warning (I think it's related to -fvisibility-inline-hidden I was fixing elsewhere some time ago, so could be fixed by enabling this flag for Assimp as well).

[bigbike]

@mosra : Thank you.
This is a simple diff. I think we are good to go. Would you like to manually push it or you would like me to do it? I am totally fine either way.

Update: I just tested it on the dev server which was linux system. I tested the system (headless mode) with Replica model (vertex colored) and glb model. Result pictures from both experiments look good to me.

[msavva]

LGTM! 👍 I like the minimalism and cleanliness now that we're relying on Utility::Resource!

[mosra]

Pushed a squashed commit w/o the .circleci changes. I will need to leave soon, so feel free to press the merge button once the CI approves :)

[bigbike]

@mosra: Thank you, Vladimir.

Also many thanks to the other reviewers. Merge and close it.