-
Notifications
You must be signed in to change notification settings - Fork 419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support ptex mesh - step 5: improve the shader and rendering code #203
Conversation
This diff is ready to be reviewed. Thanks. |
src/esp/assets/PTexMeshData.h
Outdated
void setGamma(const float& val); | ||
|
||
float saturation() const; | ||
void setSaturation(const float& val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I am working on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
.
src/esp/gfx/PTexMeshShader.cpp
Outdated
// Image size in given mip level 0 | ||
int mipLevel = 0; | ||
int widthEntry = 0; | ||
const auto width = texture.imageSize(mipLevel)[widthEntry]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that this code was already there and you just moved it, but what about texture.imageSize(mipLevel).x()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion.
src/esp/assets/PTexMeshData.h
Outdated
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely you are right. Good catch!
The code is updated, and ready to be reviewed again. Thanks. |
Please let me know your concerns on this PR. Thanks! |
@mosra : do you have more comments on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me -- I don't have the full context in my head right now, so apologies if I missed something.
|
||
// get image width in given mip level 0 | ||
int mipLevel = 0; | ||
const auto width = texture.imageSize(mipLevel).x(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msbaines: May I ask you what our latest plan is on rendering the ptex mesh on WebGL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mosra : Yeah, the geometry shader is a problem as it is not available in WebGL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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
Checklist