Support ptex mesh - step 5: improve the shader and rendering code #203

bigbike · 2019-09-11T00:42:37Z

Motivation and Context

As titled.
-) added illumination parameters, gamma, saturation etc.;
-) refactored the PTexMeshShader a bit, moving function implementations to .cpp file;
-) renamed a couple of functions in PTexMeshShader;
-) cached the uniform positions;

How Has This Been Tested

local built

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.

-) added gamma, saturation etc.; -) refactored the PTexMeshShader a bit, moving function implementations to .cpp file; -) cached the uniform positions;

bigbike · 2019-09-11T00:43:46Z

This diff is ready to be reviewed. Thanks.

mosra · 2019-09-11T22:07:18Z

src/esp/assets/PTexMeshData.h

+  void setGamma(const float& val);
+
+  float saturation() const;
+  void setSaturation(const float& val);


It would be great to add docstrings here, if nothing else at least a note about the inversion of gamma that's done inside.

Or to the shader. Or both.

OK. I am working on it.

@mosra:
I think the replicaSDK makes the code complicated by using the inverse of the gamma.
So the solution here is to remove it everywhere in the code. We only set and use "gamma".
The initial value of gamma, which was 1.6969f, is now changed to 1.0f / 1.6969f.

mosra · 2019-09-11T22:08:28Z

src/esp/gfx/PTexMeshShader.cpp

+  // Image size in given mip level 0
+  int mipLevel = 0;
+  int widthEntry = 0;
+  const auto width = texture.imageSize(mipLevel)[widthEntry];


I get that this code was already there and you just moved it, but what about texture.imageSize(mipLevel).x()?

Good suggestion.

mosra · 2019-09-11T22:14:06Z

src/esp/assets/PTexMeshData.h

@@ -64,12 +62,26 @@ class PTexMeshData : public BaseMesh {
  virtual void uploadBuffersToGPU(bool forceReload = false) override;
  virtual Magnum::GL::Mesh* getMagnumGLMesh(int submeshID) override;

+  float exposure() const;
+  void setExposure(const float& val);


Just float val, maybe? The reference doesn't save anything (is actually slower than passing by-value in this case).

Same in the two cases below.

Definitely you are right. Good catch!

bigbike · 2019-09-12T22:53:55Z

The code is updated, and ready to be reviewed again. Thanks.

src/esp/gfx/PTexMeshShader.h

bigbike · 2019-09-16T15:25:34Z

Please let me know your concerns on this PR. Thanks!

bigbike · 2019-09-17T15:58:26Z

@mosra : do you have more comments on this PR?

mosra

Looks good to me -- I don't have the full context in my head right now, so apologies if I missed something.

mosra · 2019-09-17T18:18:33Z

src/esp/gfx/PTexMeshShader.cpp

+
+  // get image width in given mip level 0
+  int mipLevel = 0;
+  const auto width = texture.imageSize(mipLevel).x();


Since the rendering uses geometry shaders, I assume it's not planned to be running on WebGL, right? (If it would be, the imageSize() would be problematic, but otherwise not at all.)

@msbaines: May I ask you what our latest plan is on rendering the ptex mesh on WebGL?

@mosra : Yeah, the geometry shader is a problem as it is not available in WebGL.

I am currently planning on using the vertex color information from the ply mesh but at some point would like to migrate to using the ptex textures. Not sure when. It will depend on whether the vertex color-only is OK. The other issue is that the textures are 100s of MB each so may not be feasible for WebGL.

Texture size could be fixed by using Basis texture compression (see mosra/magnum-plugins#62, I'm in process of integrating that), ideally the compressed textures being upstreamed back to Replica.

For vertex colors, when I checked how a PLY file looks compared to the textured GLB, it was quite underwhelming :) For human eyes and a 4K screen at least, when rendering tiny images the difference would probably be minimal.

bigbike · 2019-09-17T18:41:15Z

I will merge it to master to unblock myself this afternoon.

…cebookresearch#203) * Support ptex mesh - step 5: improve the shader and rendering code -) added gamma, saturation etc.; -) refactored the PTexMeshShader a bit, moving function implementations to .cpp file; -) cached the uniform positions; * minor * minor * resolve Ci error * rename gamma as gammaInverse in the shader * add docstring, get rid of the inverse of gamma everywhere * minor

bigbike added 3 commits September 10, 2019 17:28

Support ptex mesh - step 5: improve the shader and rendering code

3baa02a

-) added gamma, saturation etc.; -) refactored the PTexMeshShader a bit, moving function implementations to .cpp file; -) cached the uniform positions;

minor

d64ae2c

minor

021f738

bigbike requested review from mosra, msavva, erikwijmans and msbaines September 11, 2019 00:42

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 11, 2019

erikwijmans added this to the Replica PTex Rendering Support milestone Sep 11, 2019

bigbike added 2 commits September 11, 2019 14:05

resolve Ci error

e3c1c47

rename gamma as gammaInverse in the shader

08c4018

mosra reviewed Sep 11, 2019

View reviewed changes

bigbike added 2 commits September 12, 2019 15:37

add docstring, get rid of the inverse of gamma everywhere

69793f5

minor

4f52d1f

mosra reviewed Sep 13, 2019

View reviewed changes

src/esp/gfx/PTexMeshShader.h Show resolved Hide resolved

mosra approved these changes Sep 17, 2019

View reviewed changes

bigbike merged commit f8ffc9d into master Sep 17, 2019

bigbike deleted the replica_improve_shader branch September 17, 2019 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ptex mesh - step 5: improve the shader and rendering code #203

Support ptex mesh - step 5: improve the shader and rendering code #203

bigbike commented Sep 11, 2019

bigbike commented Sep 11, 2019

mosra Sep 11, 2019

bigbike Sep 11, 2019

bigbike Sep 12, 2019

mosra Sep 11, 2019

bigbike Sep 11, 2019

mosra Sep 11, 2019

bigbike Sep 11, 2019

bigbike commented Sep 12, 2019

bigbike commented Sep 16, 2019

bigbike commented Sep 17, 2019

mosra left a comment

mosra Sep 17, 2019

bigbike Sep 17, 2019

bigbike Sep 17, 2019

msbaines Sep 17, 2019

mosra Sep 18, 2019

bigbike commented Sep 17, 2019

Support ptex mesh - step 5: improve the shader and rendering code #203

Support ptex mesh - step 5: improve the shader and rendering code #203

Conversation

bigbike commented Sep 11, 2019

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

bigbike commented Sep 11, 2019

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

bigbike commented Sep 12, 2019

bigbike commented Sep 16, 2019

bigbike commented Sep 17, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbike commented Sep 17, 2019