Skip to content
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 Rendering Replica models in the simulator (under CONSTRUCTION) #132

Closed
wants to merge 39 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
e7e23ed
1st commit:
bigbike Aug 5, 2019
542d34f
pTexmeshdata
bigbike Aug 6, 2019
6885297
remove unnecessary macros
bigbike Aug 6, 2019
809ab0a
-) revert the mmap;
bigbike Aug 7, 2019
97eeb0f
save and load the adjacent faces
bigbike Aug 7, 2019
17c982f
minor
bigbike Aug 7, 2019
df7f34f
minor
bigbike Aug 7, 2019
8773ddd
Merge branch 'master' into replica
bigbike Aug 7, 2019
3ee45ed
minor
bigbike Aug 7, 2019
940df4e
Stop supporting the old format.
bigbike Aug 7, 2019
9aae2d8
minor
bigbike Aug 9, 2019
161ba3f
Merge branch 'master' into replica
bigbike Aug 9, 2019
e2ed28c
working on the shaders
bigbike Aug 12, 2019
fb665b5
format
bigbike Aug 13, 2019
57593bb
Add clear binding points for textures
bigbike Aug 14, 2019
da79cf6
Merge branch 'master' into replica
bigbike Aug 14, 2019
dd6a0ae
cache the uniform locations
bigbike Aug 14, 2019
62029e3
fix
bigbike Aug 16, 2019
9f88219
Merge branch 'master' into replica
bigbike Aug 16, 2019
0948307
merge
bigbike Aug 16, 2019
fc92467
Revert "merge"
bigbike Aug 16, 2019
cfc92e2
Merge branch 'master' into replica
bigbike Aug 20, 2019
92b35ea
merge
bigbike Aug 20, 2019
0705779
Merge branch 'master' into replica
bigbike Aug 20, 2019
d66a5d5
Almost there:
bigbike Aug 24, 2019
fd96621
Merge branch 'master' into replica
bigbike Aug 24, 2019
3c7c963
minor
bigbike Aug 26, 2019
48e00eb
use map to cache the uniform locations
bigbike Aug 26, 2019
1c6cb00
Merge branch 'master' into replica
bigbike Aug 26, 2019
a2ee4a9
Revert "use map to cache the uniform locations"
bigbike Aug 26, 2019
77ccc25
debug on macOS
bigbike Aug 27, 2019
2cf1154
Merge branch 'master' into replica
bigbike Aug 28, 2019
9fdfbdc
Merge branch 'master' into replica
bigbike Aug 28, 2019
dcd693c
Merge branch 'master' into replica
bigbike Sep 4, 2019
6ee58da
debug
bigbike Sep 4, 2019
507ec12
Merge branch 'master' into replica
bigbike Sep 6, 2019
f04d33c
Merge branch 'master' into replica
bigbike Sep 6, 2019
c901eea
Merge branch 'master' into replica
bigbike Sep 12, 2019
0de630b
Merge branch 'master' into replica
bigbike Sep 17, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/esp/assets/Asset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ AssetInfo AssetInfo::fromPath(const std::string& path) {
info.type = AssetType::INSTANCE_MESH;
} else if (endsWith(path, "ptex_quad_mesh.ply")) {
info.type = AssetType::FRL_PTEX_MESH;
} else if (endsWith(path, "mesh.ply")) {
// Warning:
// The order of if clause matters. cannot move "mesh.ply" before
// "ptex_quad_mesh.ply" or "semantic_quad_mesh.ply"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do

ASSERT(!endsWith("quad_mesh.ply"));

here instead of the comment :)

info.type = AssetType::FRL_PTEX_MESH;
} else if (endsWith(path, "house.json")) {
info.type = AssetType::SUNCG_SCENE;
} else if (endsWith(path, ".glb")) {
Expand Down
79 changes: 57 additions & 22 deletions src/esp/assets/PTexMeshData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@

#include "PTexMeshData.h"

#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <fstream>
#include <sstream>
#include <unordered_map>
#include <vector>

#include <Corrade/Containers/Array.h>
#include <Corrade/Containers/ArrayView.h>
#include <Corrade/Utility/Directory.h>
#include <Magnum/GL/BufferTextureFormat.h>
#include <Magnum/ImageView.h>
Expand All @@ -34,7 +38,10 @@ void PTexMeshData::load(const std::string& meshFile,
ASSERT(io::exists(atlasFolder));

// Parse parameters
const auto& paramsFile = atlasFolder + "/parameters.json";
// careful: when using "join", no leading forward slash in the file name.
// otherwise the corrade would think it is absolute path
bigbike marked this conversation as resolved.
Show resolved Hide resolved
const auto& paramsFile =
Corrade::Utility::Directory::join(atlasFolder, "parameters.json");
ASSERT(io::exists(paramsFile));
const io::JsonDocument json = io::parseJsonFile(paramsFile);
splitSize_ = json["splitSize"].GetDouble();
Expand Down Expand Up @@ -203,20 +210,23 @@ void PTexMeshData::calculateAdjacency(const PTexMeshData::MeshData& mesh,

std::unordered_map<uint64_t, std::vector<EdgeData>> edgeMap;

size_t numFaces = mesh.ibo.size() / 4;
// only works on quad meshes
const int polygonStride = 4;
size_t numFaces = mesh.ibo.size() / polygonStride;

typedef std::unordered_map<uint64_t, std::vector<EdgeData>>::iterator
EdgeIter;
std::vector<EdgeIter> edgeIterators(numFaces * 4);
std::vector<EdgeIter> edgeIterators(numFaces * polygonStride);

// for each face
for (int f = 0; f < numFaces; f++) {
// for each edge
for (int e = 0; e < 4; e++) {
for (int e = 0; e < polygonStride; e++) {
// add to edge to face map
const int e_index = f * 4 + e;
const int e_index = f * polygonStride + e;
const uint32_t i0 = mesh.ibo[e_index];
const uint32_t i1 = mesh.ibo[f * 4 + ((e + 1) % 4)];
const uint32_t i1 =
mesh.ibo[f * polygonStride + ((e + 1) % polygonStride)];
const uint64_t key =
(uint64_t)std::min(i0, i1) << 32 | (uint32_t)std::max(i0, i1);

Expand All @@ -236,11 +246,11 @@ void PTexMeshData::calculateAdjacency(const PTexMeshData::MeshData& mesh,
}
}

adjFaces.resize(numFaces * 4);
adjFaces.resize(numFaces * polygonStride);

for (int f = 0; f < numFaces; f++) {
for (int e = 0; e < 4; e++) {
const int e_index = f * 4 + e;
for (int e = 0; e < polygonStride; e++) {
const int e_index = f * polygonStride + e;
auto it = edgeIterators[e_index];
const std::vector<EdgeData>& adj = it->second;

Expand All @@ -267,13 +277,16 @@ void PTexMeshData::calculateAdjacency(const PTexMeshData::MeshData& mesh,
}

// pack adjacent face and rotation into 32-bit int
adjFaces[f * 4 + e] = (rot << ROTATION_SHIFT) | (adjFace & FACE_MASK);
adjFaces[f * polygonStride + e] =
(rot << ROTATION_SHIFT) | (adjFace & FACE_MASK);
}
}
}

void PTexMeshData::loadMeshData(const std::string& meshFile) {
PTexMeshData::MeshData originalMesh;

std::cout << "start parsing PLY... " << std::endl;
parsePLY(meshFile, originalMesh);

submeshes_.clear();
Expand Down Expand Up @@ -583,7 +596,7 @@ void PTexMeshData::uploadBuffersToGPU(bool forceReload) {
currentMesh->ibo.setData(submeshes_[iMesh].ibo,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mosra:
I am debugging the segfault.
After this "setData", do you think currentMesh->ibo.size() would equal to submeshes_[iMesh].ibo.size()?
Same for the vbo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am asking this question is because I observed the size of currentMesh->ibo.size() was 4x of the submeshes_[iMesh].ibo.size().
I suspect currentMesh->ibo.size() returns the bytes of the entire array.
If this is the case, later "setCount(currentMesh->ibo.size())" would be wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this "setData", do you think currentMesh->ibo.size() would equal to submeshes_[iMesh].ibo.size()?

The naming is making this harder than it should be, first I assumed the two ibos are both std::vectors but they're actually not :) Calling GL::Buffer::size() returns a number of bytes, since the buffer is a typeless piece of memory. And it's a query into GL, which is best avoided (moreover this function is not available at all in OpenGL ES or WebGL, but I'm not sure if that matters here) -- if you have the same information available CPU-side, it's recommended to prefer using that -- the size(), data(), imageSize(), ... functions on GL objects are meant mainly for debugging purposes.

Few lines below in the diff I see this -- could it be related?

-        .setCount(currentMesh->ibo.size() / 2)
+        .setCount(currentMesh->ibo.size())

Copy link
Contributor Author

@bigbike bigbike Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the problem. We actually need to pass the "number of primitives" to the setCount(), not the "number of bytes". That is why no matter if we divide it by 2, it will segfault anyway.

Thank you for the confirmation. Yes, I also consider using the info from CPU-side, like you mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any other example in your docs that shows how to correctly use ibo, vbo?
The "textured triangle" demo is a bit too simple in this case, in my opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Primitives example expands on that, and the GL::Mesh documentation is very extensive.

Magnum::GL::BufferUsage::StaticDraw);
}
std::cout << "... done" << std::endl;
std::cout << "done" << std::endl;

std::cout << "Calculating mesh adjacency... ";
std::cout.flush();
Expand All @@ -594,6 +607,7 @@ void PTexMeshData::uploadBuffersToGPU(bool forceReload) {
for (int iMesh = 0; iMesh < submeshes_.size(); ++iMesh) {
calculateAdjacency(submeshes_[iMesh], adjFaces[iMesh]);
}
std::cout << "... done" << std::endl;

for (int iMesh = 0; iMesh < submeshes_.size(); ++iMesh) {
auto& currentMesh = renderingBuffers_[iMesh];
Expand All @@ -602,35 +616,56 @@ void PTexMeshData::uploadBuffersToGPU(bool forceReload) {
currentMesh->abo);
currentMesh->abo.setData(adjFaces[iMesh],
Magnum::GL::BufferUsage::StaticDraw);

// experiment code (may not work):
currentMesh->mesh.setPrimitive(Magnum::GL::MeshPrimitive::LinesAdjacency)
.setCount(currentMesh->ibo.size() / 2)
.addVertexBuffer(currentMesh->vbo, 0, gfx::PTexMeshShader::Position{})
.setIndexBuffer(currentMesh->ibo, 0,
Magnum::GL::MeshIndexType::UnsignedInt);
}

// load atlas data and upload them to GPU

std::cout << "loading atlas textures: " << std::endl;
for (size_t iMesh = 0; iMesh < renderingBuffers_.size(); ++iMesh) {
const std::string rgbFile =
atlasFolder_ + "/" + std::to_string(iMesh) + "-color-ptex.rgb";
if (!io::exists(rgbFile)) {
ASSERT(false, "Can't find " + rgbFile);
}
const std::string rgbFile = Corrade::Utility::Directory::join(
atlasFolder_, std::to_string(iMesh) + "-color-ptex.rgb");

ASSERT(io::exists(rgbFile), Error : Cannot find the rgb file);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's CORRADE_ASSERT() btw, which could make esp/logging.h obsolete -- and it supports printing everything that Utility::Debug can :)

Suggested change
ASSERT(io::exists(rgbFile), Error : Cannot find the rgb file);
CORRADE_ASSERT(io::exists(rgbFile), "Error : Cannot find the rgb file");

Also, for io::exists there's Directory::exists() ... and and and

... 🤔 should I open a PR moving everything from esp/logging.h and io/io.h to builtin Corrade functionality? 😄 (cc: @msavva @erikwijmans)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logging.h and io.h are legacy.
I totally agree to move them to Corrade. But I do not think it is that urgent.


std::cout << "\rLoading atlas " << iMesh + 1 << "/"
<< renderingBuffers_.size() << "... ";
<< renderingBuffers_.size() << " from " << rgbFile << "... ";
std::cout.flush();

Cr::Containers::Array<const char, Cr::Utility::Directory::MapDeleter> data =
Cr::Utility::Directory::mapRead(rgbFile);
const int dim = static_cast<int>(std::sqrt(data.size() / 3)); // square
const size_t numBytes = io::fileSize(rgbFile);
Copy link
Contributor Author

@bigbike bigbike Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to load the textures from the file. Such textures are not in common picture format (e.g., png, jpg, tga), but some customized format in binary.

In ReplicaSDK, they mapped a texture (rgbFile) to the memory, and uploaded it to a texture (atlas). The code is as follows.

const size_t numBytes = std::experimental::filesystem::file_size(rgbFile);
// Open file
int fd = open(std::string(rgbFile).c_str(), O_RDONLY, 0);
void* mmappedData = mmap(NULL, numBytes, PROT_READ, MAP_PRIVATE | MAP_POPULATE, fd, 0);
meshes[i]->atlas.Upload(mmappedData, GL_RGB, GL_UNSIGNED_BYTE);
munmap(mmappedData, numBytes);
close(fd);

@mosra : Do you think the current implementation is proper?
Do you have better ideas in terms of leveraging Magnum to do the same work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait wait but this is practically reverting #106 to what was there before, and that change was done in order to help with #84 (and besides that, reducing platform-specific code). The code you're showing is precisely what mapRead() does if I see correctly, apart from the non-portable MAP_POPULATE flag

Is there any issue with the way it was done using Directory::mapRead()? Because in my testing I couldn't see any problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow me to reply this one first before I read the other comments.
I found an issue in the previous code that the "dim" was wrong, which resulted from "numBytes".

After reading your comment, assuming the mapRead() did exactly the same, then it means getting the "numBytes" using "data.size()" is incorrect.

What is the correct way to get this number?

I would change it back.

(I am not aware of the #106 or #84 since I was on leave for a couple of months. Good to know.)

Thank you very much for the comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. The array returned from mapRead() should have its size equal to number of bytes in that file (and my tests so far were confirming that). The rest of the code looks exactly the same, so that would mean mapRead() returns a wrong size?

What does numBytes give and what data.size()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. It is also possible my testing code has some bugs.
Allow me to take another look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested it again. The numbers are identical. There must be something wrong in my previous debugging code. My bad. I sincerely apologize for the confusion. Sorry, Vladimir.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem ;) In any case, I'm not claiming my implementation is bug-free either, so if you see something strange with it, let me know.

const int dim = static_cast<int>(std::sqrt(numBytes / 3)); // square

// Open file
int fd = open(std::string(rgbFile).c_str(), O_RDONLY, 0);
// MAP_POPULATE does not work on mac. It is to reduce the penalty of page
// faults. Code should be OK without it. void* mmappedData = mmap(NULL,
// numBytes, PROT_READ, MAP_PRIVATE | MAP_POPULATE, fd, 0);
void* mmappedData = mmap(NULL, numBytes, PROT_READ, MAP_PRIVATE, fd, 0);

Corrade::Containers::ArrayView<unsigned char> data(
(unsigned char*)(mmappedData), numBytes);

// atlas
// the size of each image is dim x dim x 3 (RGB), which equals to numBytes
Magnum::ImageView2D image(Magnum::PixelFormat::RGB8UI, {dim, dim}, data);
renderingBuffers_[iMesh]
->tex.setWrapping(Magnum::GL::SamplerWrapping::ClampToEdge)
.setMagnificationFilter(Magnum::GL::SamplerFilter::Linear)
.setMinificationFilter(Magnum::GL::SamplerFilter::Linear)
// .setStorage(1, GL::TextureFormat::RGB8UI, image.size())
// .setStorage(1, Magnum::GL::TextureFormat::RGB8UI, image.size())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mosra :
The code in the master version commented it out.
Do I need to set the storage here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was commited this way by @msavva in October 2018, I don't know what was the original reason :) Generally, calling setSubImage() without setStorage() or setImage() being called first will result in a GL error and nothing being done, so I guess that's what is happening here? (Unless the texture storage is being set up somewhere else, but I don't see any such code anywhere else.)

If you are on linux you can run the viewer with --magnum-gpu-validation on and it'll tell you in the console output about any GL errors. (Mac doesn't implement GL debug output, so there you'd need to test for GL::Renderer::error() manually by hand.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am developing everything on the Mac. I guess I will have to require a linux box. :)

So here, if I understand you correctly, I should actively call setStorage(). Correct?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, or change the below to setImage() ... but setStorage() is the preferred thing to do.

.setSubImage(0, {}, image);

munmap(mmappedData, numBytes);
close(fd);

std::cout << "done" << std::endl;
}
std::cout << "... done" << std::endl;

buffersOnGPU_ = true;
}
Expand Down
19 changes: 17 additions & 2 deletions src/esp/assets/PTexMeshData.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ class PTexMeshData : public BaseMesh {

// ==== geometry ====
void load(const std::string& meshFile, const std::string& atlasFolder);
float exposure() const;
void setExposure(const float& val);

uint32_t tileSize() const { return tileSize_; }

const std::vector<MeshData>& meshes() const;
Expand All @@ -61,13 +60,29 @@ 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);

// TODO:
float gamma() const;
void setGamma(const float& val);

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

protected:
void loadMeshData(const std::string& meshFile);

float splitSize_ = 0.0f;
uint32_t tileSize_ = 0;

float exposure_ = 1.0f;
float gamma_ = 1.0f;
float saturation_ = 1.0f;
bool isHdr_ = false;

std::string atlasFolder_;

std::vector<MeshData> submeshes_;

// ==== rendering ====
Expand Down
15 changes: 11 additions & 4 deletions src/esp/assets/ResourceManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,21 @@ bool ResourceManager::loadPTexMeshData(const AssetInfo& info,
// if this is a new file, load it and add it to the dictionary
const std::string& filename = info.filepath;
if (resourceDict_.count(filename) == 0) {
const std::string atlasDir =
Corrade::Utility::String::stripSuffix(filename, "ptex_quad_mesh.ply") +
"ptex_textures";
const auto atlasDir = [=]()->std::string{
// backwards compatibility
if (Corrade::Utility::String::endsWith(filename, "ptex_quad_mesh.ply")) {
return (Corrade::Utility::String::stripSuffix(filename, "ptex_quad_mesh.ply") +
"ptex_textures");
}
// officially released Replica dataset
return (Corrade::Utility::String::stripSuffix(filename, "mesh.ply") +
"textures");
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why the lambda is needed if it's used just on a single place :) Also there's Directory::path() to make this an oneliner:

const std::string atlasDir = Corrade::Utility::Directory::join(
  Corrade::Utility::Directory::path(filename), "textures");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new format is using "textures" as the folder name, while the old one was using "ptex_textures". Cannot put them in a single line.

Well, I can use Ternary Operator instead of the lambda.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh well, sorry, missed that part, thought it's just cutting off a different filename 🙈


meshes_.emplace_back(std::make_unique<PTexMeshData>());
int index = meshes_.size() - 1;
auto* pTexMeshData = dynamic_cast<PTexMeshData*>(meshes_[index].get());
pTexMeshData->load(filename, atlasDir);
pTexMeshData->load(filename, atlasDir());

// update the dictionary
resourceDict_.emplace(filename, MeshMetaData(index, index));
Expand Down
1 change: 1 addition & 0 deletions src/esp/core/logging.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
if (!(x)) { \
std::cout << "Assert failed: " #x << ", " << __FILE__ << ":" << __LINE__ \
<< std::endl; \
std::cout << #__VA_ARGS__ << std::endl; \
exit(-1); \
} \
} while (false)