Save 25% of memory usage on vertex position buffer (PTex Mesh) #348
Conversation
facebook-github-bot
added
the
CLA Signed
|
This PR is ready to be reviewed. Thanks. |
|
Will the shader need to change here: https://github.com/facebookresearch/habitat-sim/blob/master/src/shaders/ptex-default-gl410.vert#L1 ? |
Oh, this is because OpenGL will fill the 4th component, w, with 1.0 automatically for the user if only a 3-dimensional vertex position is provided. I did not realize it would cause confusion here. If you really feel bad about it, maybe I can change it to vec3. Then the following code will be modified as: @mosra: Do you think it is necessary? |
|
No, keep the |
| @@ -665,7 +665,7 @@ void PTexMeshData::parsePLY(const std::string& filename, | |||
| "not greater than 0", ); | |||
| } | |||
|
|
|||
| meshData.vbo.resize(numVertices, vec4f(0, 0, 0, 1)); | |||
| meshData.vbo.resize(numVertices, vec3f(0, 0, 0)); | |||
There was a problem hiding this comment.
You should assert that positionDimensions < 4.
| @@ -22,7 +22,7 @@ namespace assets { | |||
| class PTexMeshData : public BaseMesh { | |||
| public: | |||
| struct MeshData { | |||
| std::vector<vec4f> vbo; | |||
| std::vector<vec3f> vbo; | |||
| std::vector<vec4f> nbo; | |||
There was a problem hiding this comment.
Couldn't nbo also be a vec3?
There was a problem hiding this comment.
As far as I can see, neither nbo nor cbo is used right now (only populated). Is that correct?
There was a problem hiding this comment.
As far as I can see, neither
nbonorcbois used right now (only populated). Is that correct?
Yes, you are right. I checked them as well before this PR.
Ah, I didn't realize OpenGL would do that. How nice of it :) No need to change it as people will rarely actually look at the shaders themselves, I just wanted to check for my own knowledge. |
|
You may be able to get a really nice speedup if you used the mmapped buffer directly instead of copying vertex attributes a vertex at a time. |
|
@msbaines That's precisely what I'm working on to enable with the asset import rework (happening in mosra/magnum#371 -- description might be a bit outdated tho). Together with that we could get rid of all the copies of data stored CPU-side. |
For vertices at least, we do need CPU-side copies for #333 |
Ah. Very good point. I already broke this once optimizing mesh loading:) You could still keep a CPU-side copy around with mmap. You'd just need to manage the file handle and mapping. We really need to write tests for this behavior to avoid breaking it. |
Codecov Report
@@ Coverage Diff @@
## master #348 +/- ##
==========================================
- Coverage 56.72% 56.71% -0.01%
==========================================
Files 151 151
Lines 6740 6741 +1
==========================================
Hits 3823 3823
- Misses 2917 2918 +1
Continue to review full report at Codecov.
|
…ookresearch#348) * Save 25% of memory usage on vertex position buffer * minor
Motivation and Context
As titled.
Using Vector3 instead of Vector4 for a vertex position in PTex mesh
Reduce memory footprint
How Has This Been Tested
local test with utility viewer.
Types of changes
Checklist