Configurable Shader and ShaderManager initial setup #450

matthewjmay · 2020-01-27T22:19:03Z

Motivation and Context

Initial PR for issue #319.

How Has This Been Tested

New test cases will be added incrementally for the configurable shaders project as real functionality is implemented. For now, this code isn't being used by existing render pipeline.
All existing test cases pass.

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.

codecov-io · 2020-01-28T04:47:39Z

Codecov Report

Merging #450 into master will decrease coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #450      +/-   ##
==========================================
- Coverage   58.55%   58.36%   -0.19%     
==========================================
  Files         165      162       -3     
  Lines        7327     7155     -172     
  Branches       84       84              
==========================================
- Hits         4290     4176     -114     
+ Misses       3037     2979      -58

Flag	Coverage Δ
#CPP	`53.11% <100%> (-0.53%)`	⬇️
#JavaScript	`10% <ø> (ø)`	⬆️
#Python	`79.57% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
src/esp/sim/Simulator.h	`100% <ø> (ø)`	⬆️
src/esp/gfx/ShaderManager.h	`100% <100%> (ø)`
src/esp/bindings/OpaqueTypes.h
src/esp/sensor/RedwoodNoiseModel.h
src/esp/bindings/SceneBindings.cpp
src/esp/bindings/SensorBindings.cpp
src/esp/sensor/VisualSensor.h	`20% <0%> (+8.23%)`	⬆️
src/esp/sensor/Sensor.h	`81.81% <0%> (+12.58%)`	⬆️
src/esp/scene/SemanticScene.h	`30.76% <0%> (+14.44%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74d8501...b326caf. Read the comment docs.

aclegg3

This looks like a good pass at the Shader/ShaderManager API based on phase 1 of the plan. 😀 At this point the implementation is not functional within the pipeline, so my comments are pretty high-level. I'd like to see what others think about this.

src/esp/gfx/ShaderManager.cpp

aclegg3 · 2020-01-29T19:28:26Z

src/esp/gfx/ShaderManager.cpp

+}
+
+bool ShaderManager::deleteShader(const std::string& id) {
+  return shaders_.erase(id);


Does this have repercussions with DrawableGroups possibly using the Shader? Can/should we check for that here or what will the behavior be if I delete a Shader which is in use?

Shaders will be stored in DrawableGroups as shared pointers, so this won't be a problem. The only repercussion is that if a new shader is made with the same name (which was deleted), DrawableGroups using the old shader won't be updated with the new shader.

aclegg3 · 2020-01-29T19:42:14Z

src/esp/gfx/Shader.h

+class RenderCamera;
+
+// TODO: string type to allow for dynamically added shader types
+enum class ShaderType {


This duplicates the enum in ResourceManager. That will be removed, this header included there, and existing Shader setup replaced with this pipeline in later phase 1 PRs, right?

that's exactly right

aclegg3 · 2020-01-29T19:43:28Z

src/esp/gfx/Shader.h

+
+struct ShaderConfiguration {
+  ShaderType type = ShaderType::TEXTURED_SHADER;
+  // JSON options;


Derived configs for shader specific parameters?

There are 3 options here:

Single configuration which has all info needed for any shader factory method (bad, not extensible, massive struct).

Some type that can represent any settings (ie. json, boost::variant, c++17's std::any) (we only have json included in habitat right now, and would need to define complex conversion operators)

A base class which gets casted down the its actual type by factory methods

Talked offline with @aclegg3, we agreed that the best option is likely 3. Note that these conversions will only occur when creating a brand new shader or changing the configuration of an existing one, which is likely not very often, and definitely not in the render pipeline

aclegg3 · 2020-01-29T19:50:07Z

src/esp/gfx/Shader.h

+class DrawableGroup;
+class RenderCamera;
+
+// TODO: string type to allow for dynamically added shader types


There won't be dynamically added types, right? Rather, we allow dynamically added configurations of supported types.

I was thinking we were going to allow for dynamically added Shader types to be defined and added in python/javascript, similar to what we are doing with the Sensor factory

aclegg3 · 2020-01-29T20:17:04Z

src/esp/gfx/Shader.cpp

+      break;
+  }
+
+  /* Default setup for Phong, shared by all models */


Will all Shaders of same shaderProgram type be collapsed into one setup? (e.g. COLORED_SHADER_PHONG and TEXTURED_SHADER_PHONG different only in config, not separately sub-classed)

Yep, currently we should just have Flat, PrimitiveID, Phong, and PtexMesh ShaderProgram classes, and COLORED_SHADER_PHONG and TEXTURED_SHADER_PHONG will only differ in config

aclegg3 · 2020-01-29T20:26:20Z

src/esp/sim/Simulator.h

@@ -308,6 +311,8 @@ class Simulator {

  gfx::WindowlessContext::uptr context_ = nullptr;
  std::shared_ptr<gfx::Renderer> renderer_ = nullptr;
+  gfx::ShaderManager shaderManager_;


Stored at this level, we will likely have to pass this down into the loadScene() pipeline also to create the correct default shaders for the scene DrawableGroups based on AssetInfo types. It will also change the C++ test setup pipeline at that point.

For now, I was planning on passing this into the ResourceManager for use when drawables are created.

mosra

Didn't go in-depth yet, just general high-level comments.

(Reviewed 2 days ago but forgot to hit Submit. I need to clean up the 170 open tabs in my browser 😅 )

mosra · 2020-01-28T10:21:52Z

src/esp/gfx/Shader.h

+class DrawableGroup;
+class RenderCamera;
+
+// TODO: string type to allow for dynamically added shader types


I have to admit I'm not a big fan of doing string ops in the render loop :)

As an alternative, Magnum provides ResourceKey that's a 32/64-bit hash of a string, so guaranteed constant-time operation with no allocations when doing a lookup. Of course it's an one-way operation so getting the inverse (hash->string) is non-trivial, but I don't think such operation is needed very often -- instead, you'd make all getters and queries accept a ResourceKey instead of a const std::string&.

Great idea, we can definitely use that!

I wanted to clarify, these strings won't actually be used in the render loop, instead only used on new shader creation or setting a new config for a created Shader. ShaderType only exists so that we know which factory function to call when given a ShaderConfig. The actual shader lookup based on shader UUID will be done in ShaderManager (see #454).

mosra · 2020-01-28T11:04:01Z

src/esp/gfx/Shader.h

+   * configuration
+   */
+  bool prepareForDraw(const DrawableGroup& drawables,
+                      const RenderCamera& camera);


I have to admit I'm not a fan of return types indicating a failure and the caller being responsible for checking those :) What about doing the inverse -- i.e., instead of a shader being passed random drawable groups (that might, in the worst case, be differently incompatible each frame), a DrawableGroup wrapper having attached a shader? That way it's much less likely there will be a mixup -- you'd set up the group/shader connection up-front, having a clear idea what goes together.

On the other hand, in Magnum I deliberately support shaders being interchangeable -- so for example a scene can be drawn with Phong, then switched to a flat shading for texture inspection, or drawn with a MeshVisualizer on top to highlight wireframe / normal direction etc. To achieve that with the above, a drawable group could have one or more compatible shaders attached.

Another idea re error checking that I'm using quite often all over the place -- the functions asserting that given preconditions are met, plus an additional isConfigurationCompatible() (for example), that gives back a bool indicating whether a call to setConfiguration() would work (or would assert), potentially printing a more detailed diagnostic. This way there's no need for doing verbose explicit return code checking in code that you know is right (when setting up builtin shaders for scenes with known features), but on the other hand you have the ability to do explicit error checking when external factors are involved (for example when supporting custom user-supplied shaders in the python code).

Oops, no idea why I'm actually passing through DrawableGroup here. To clarify for everyone, this function exists to use the currently set config + camera to set up common shader parameters which may be dependent on camera (ie. projectionMatrix, lightposition, etc). Since we can do configuration checking in setConfiguration, the return type can be void here, will update.

As for the setConfiguration idea, any ideas on how to check isConfigurationCompatible in a clean way without actually creating a ShaderProgram? I.e. different shader types will have different valid configurations, so checking validity would require creating a new shader via factory (since Shader doesn't know about the concrete ShaderProgram types), unless I had a separate entry in the lookup table for validation, which I think is overly complicated.

I have to admit I was not fully involved in the design discussion for this -- what's the actual definition of a "compatible configuration"? Or how custom can the custom shaders be?

Let's say a custom shader would need an additional buffer texture containing some per-face data to do some fancy shading. Or a PBR shader that additionally needs a metalness texture, or an instanced shader that supplies per-instance matrices via an additional attribute. How would one express that via the configuration? What's the scope of the configuration? Key/value pairs or more? What is a possible use case and what is already out of reach?

I'd say that for a flexible shader management you'd need to:

have a possibility to call something up-front, before drawing everything that belongs to given shader, for a global setup

have a possibility to call something before each draw, for a per-mesh setup

In other words, for each new shader, letting users implement their own Drawable that gets used with it, and letting them put things into / operate drawable groups directly. That doesn't need any configuration or configuration validation because users are in charge of that, and ... isn't that kinda already there? I fear this ends up overengineered without really providing the flexibility needed.

Thanks for these questions, we definitely need to get the design right before moving further.

The idea is that the "configuration" will specify attributes for shared drawable group setup for any arbitrary type of Shader. A possible use case of this configuration may be to specify type "phong" with attributes numLights, positions, shininess, etc, or type "flat" with texture flags, vertex colored flags, etc. Now, this Shader wrapper class does not know about these underlying shader types, so it must just accept a base type, and the underlying shaderprogram must define what is valid.

So it seems strange that we have the Shader wrapper class at all, since we could just allow the client code to configure the underlying ShaderProgram directly, before passing it to a DrawableGroup to be used. However, the advantages of this wrapper are:

The client code doesn't need to know about the underlying shader types being used, then they can just easily pass us a config and we handle shader setup and reuse. Phong implementation could even be swapped out depending on what's registered in the factory.

The client code doesn't need to store and keep track of their configs (ie. just set and forget, if I want to get it later, and possibly modify it, I can access it with getConfiguration

The configuration can be stored/read in material lighting information in assets (ie. GLtf), instead of always having to be created programmatically.

The modification/reuse of ShaderPrograms is abstracted. The user shouldn't have to know that adding a light to a phong shader requires me to create a brand new ShaderProgram and set it, but if I just want to move a light I can reuse the existing ShaderProgram. With this wrapper class, they change the configuration in a single uniform way with setConfiguration. If necessary, the factories will create a new ShaderProgram which supports this new conf. If one already exists (ie. all phong shaders using the same flags and number of lights), we don't need to do anything.

I apologize if any of this isn't clear, its hard to split up the PR's into bite sized chunks that individually make sense, so please let me know if you have questions. Thanks again for the feedback.

mosra · 2020-01-28T11:06:09Z

src/esp/gfx/Shader.h

+   *
+   * @return Whether the specified @ref Drawable was drawn
+   */
+  bool draw(const Drawable& drawable);


Why render just one Drawable, tho? The main reason why the group renders everything together is that it can calculate all transforms at once and save unnecessary recalculation deep hierarchies compared to drawing one by one. (Plus also leaving room for upcoming instanced drawing etc.)

OTOH, seeing #454, maybe you are already moving away from this approach?

mosra · 2020-01-28T11:07:39Z

src/esp/gfx/ShaderManager.h

+   * same ID already exists.
+   */
+  template <typename... ShaderArgs>
+  Shader::ptr createShader(std::string id, ShaderArgs&&... args);


why not accept a std::unique_ptr<GL::AbstractShaderProgram>&& or something like that here? That way you wouldn't need any template.

I wanted to be flexible in accepting a shader config here, or copy/move an existing shader into the map.

src/esp/gfx/ShaderManager.cpp

mosra · 2020-01-31T18:55:14Z

src/esp/gfx/Shader.cpp

    ShaderType type,
    const ShaderConfiguration& cfg) {
  switch (type) {
    case ShaderType::INSTANCE_MESH_SHADER: {
-      return std::make_unique<gfx::PrimitiveIDShader>();
+      static auto primitiveShader = std::make_shared<gfx::PrimitiveIDShader>();


Not sure about the static here -- this will mean the instance survives even after the GL context is destroyed, and then when creating a new GL context, this will still return the stale, no longer valid instance.

Same with multithreading -- in case someone would want to run multiple separate simulator instances on separate threads, this makes that impossible.

Great catch, instead we will have to instantiate the shader factory as a member of the ShaderManager, and inject it into this shader class, to ensure instances are torn down correctly, and are always unique across simulator instances. I'll leave this for now, but update this in the PR that actually implements these shader factory methods properly

This can't stay unfortunately. It will break things immediately. Tearing down and re-spinning up the simulator is a very common occurrence.

removed, I'll implement shader reuse later

erikwijmans

See comment about the static

matthewjmay · 2020-02-04T00:31:09Z

Just merged in master to allow DrawableGroup to actually use the new Shader class

matthewjmay · 2020-02-25T22:01:15Z

irrelevant after #489

Added missing API->Lab changes

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jan 27, 2020

matthewjmay requested review from aclegg3 and bigbike January 27, 2020 22:37

bigbike requested review from erikwijmans, mosra and msbaines January 27, 2020 23:53

aclegg3 mentioned this pull request Jan 29, 2020

Inserted physics models are invisible #459

Closed

aclegg3 reviewed Jan 29, 2020

View reviewed changes

mosra reviewed Jan 30, 2020

View reviewed changes

mosra reviewed Jan 31, 2020

View reviewed changes

erikwijmans suggested changes Jan 31, 2020

View reviewed changes

matthewjmay added 8 commits January 31, 2020 17:58

Shader manager

c9d8cf5

Shader and ShaderManager

112aaba

silence inline "using namespace" warning

7236b88

allow multiple argument shader construction

f2dbea7

better error handling on shader creation

5ecdc88

make shader design and comments clearer

572a26c

remove static shaders

572134e

move templated func to header

336d08c

matthewjmay force-pushed the shader-manager branch from d66314b to 336d08c Compare February 1, 2020 01:59

matthewjmay added 3 commits February 3, 2020 15:45

inline templated createShader

b326caf

Merge branch 'master' into shader-manager

0b74bb9

update DrawableGroup to use real Shader class

00fd4d6

fix build failure

9b8eb9e

matthewjmay mentioned this pull request Feb 4, 2020

[DRAFT][WIP] Enable Phong shading + dynamic shader/drawable group selection #471

Closed

11 tasks

matthewjmay closed this Feb 25, 2020

matthewjmay deleted the shader-manager branch March 17, 2020 22:59

luoj1 pushed a commit to luoj1/habitat-sim that referenced this pull request Aug 16, 2022

Added missing API->Lab changes (facebookresearch#450)

e6b7f1e

Added missing API->Lab changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable Shader and ShaderManager initial setup #450

Configurable Shader and ShaderManager initial setup #450

matthewjmay commented Jan 27, 2020

codecov-io commented Jan 28, 2020 •

edited

Loading

aclegg3 left a comment

aclegg3 Jan 29, 2020

matthewjmay Jan 29, 2020

aclegg3 Jan 29, 2020

matthewjmay Jan 29, 2020

aclegg3 Jan 29, 2020

matthewjmay Jan 29, 2020 •

edited

Loading

aclegg3 Jan 29, 2020

matthewjmay Jan 29, 2020

aclegg3 Jan 29, 2020

matthewjmay Jan 29, 2020

aclegg3 Jan 29, 2020

matthewjmay Jan 29, 2020

mosra left a comment

mosra Jan 28, 2020

matthewjmay Jan 30, 2020

mosra Jan 28, 2020

matthewjmay Jan 30, 2020 •

edited

Loading

mosra Jan 31, 2020

matthewjmay Jan 31, 2020

mosra Jan 28, 2020

mosra Jan 28, 2020

matthewjmay Jan 30, 2020

mosra Jan 31, 2020

matthewjmay Jan 31, 2020 •

edited

Loading

erikwijmans Jan 31, 2020

matthewjmay Jan 31, 2020

erikwijmans left a comment

matthewjmay commented Feb 4, 2020

matthewjmay commented Feb 25, 2020

Configurable Shader and ShaderManager initial setup #450

Configurable Shader and ShaderManager initial setup #450

Conversation

matthewjmay commented Jan 27, 2020

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

codecov-io commented Jan 28, 2020 • edited Loading

Codecov Report

aclegg3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjmay Jan 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjmay Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjmay Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikwijmans left a comment

Choose a reason for hiding this comment

matthewjmay commented Feb 4, 2020

matthewjmay commented Feb 25, 2020

codecov-io commented Jan 28, 2020 •

edited

Loading

matthewjmay Jan 29, 2020 •

edited

Loading

matthewjmay Jan 30, 2020 •

edited

Loading

matthewjmay Jan 31, 2020 •

edited

Loading