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

Vertices in LoadObj is incorrect when TINYOBJLOADER_USE_DOUBLE is on #231

Closed
jasjuang opened this issue Nov 6, 2019 · 7 comments
Closed

Comments

@jasjuang
Copy link
Contributor

jasjuang commented Nov 6, 2019

Given below minimal obj file

test.obj

v -126.4800033569 12.4945001602 -173.8119964600
v -126.6900024414 21.4505004883 -170.0980072021
v -108.1169967651 -28.3661003113 -170.7310028076
f 1 2 3

Please see below minimal code

void ReadOBJ(const std::string &obj_file,
             std::vector<double> &vert_pos,
             std::vector<int> &tri_ind){
  tinyobj::attrib_t attrib;
  std::vector<tinyobj::shape_t> shapes;
  std::vector<tinyobj::material_t> materials;
  std::string warn;
  std::string err;

  tinyobj::LoadObj(&attrib, &shapes, &materials, &warn, &err, obj_file.c_str());

  for (double vertice : attrib.vertices){
    vert_pos.emplace_back(vertice);

    std::cout << vertice << std::endl;
  }

  for (auto &shape : shapes){
    for (auto &indice : shape.mesh.indices){
      tri_ind.emplace_back(indice.vertex_index);
    }
  }
}

int main(){
  std::vector<double> vert_pos;
  std::vector<int> tri_ind;

  ReadOBJ("test.obj", vert_pos, tri_ind);

  std::cout << "total verts: " << vert_pos.size() << std::endl;
  return 0;
}

If TINYOBJLOADER_USE_DOUBLE is set to on in CMake while compiling tinyobjloader, the output is all wrong as shown below:

3.68883e+19
-3.49406
1.08357e-19
2.64045
-3.68995e+19
-3.58948
-nan
-3.49488
7.39465e-42
2.83516
-3.68897e+19
-3.58222
-3.68878e+19
-3.42233
-2.00139
-2.94322
3.68922e+19
-3.58346
total verts: 18

whereas if TINYOBJLOADER_USE_DOUBLE is set to off in CMake while compiling tinyobjloader, the output is as expected like below:

-126.48
12.4945
-173.812
-126.69
21.4505
-170.098
-108.117
-28.3661
-170.731
total verts: 9

@syoyo @noma please take a look, many thanks.

@syoyo
Copy link
Collaborator

syoyo commented Nov 6, 2019

At least compiling loader_example.cc with -DTINYOBJLOADER_USE_DOUBLE=1 works fine.

Have you defined TINYOBJLOADER_USE_DOUBLE for each tiny_obj_loader.h inclusion?

#define TINYOBJLOADER_USE_DOUBLE
#include "tiny_obj_loader.h"

@jasjuang
Copy link
Contributor Author

jasjuang commented Nov 7, 2019

Thanks for the pointer. Adding target_compile_definitions(${PROJECT_NAME} PUBLIC TINYOBJLOADER_USE_DOUBLE) in the downstream CMakeLists actually does solve it for me, but why is this required when the CMake option TINYOBJLOADER_USE_DOUBLE is already set to on in the upstream?

@syoyo
Copy link
Collaborator

syoyo commented Nov 7, 2019

https://github.com/syoyo/tinyobjloader/blob/master/tiny_obj_loader.h#L130

real_t is ifdef'ed at header level, so you need to define TINYOBJLOADER_USE_DOUBLE for each tiny_obj_loader.h inclusion.

Using templated type for real_t will solve such a confusion #233

@jasjuang
Copy link
Contributor Author

jasjuang commented Nov 7, 2019

Thanks. Looking forward to a solution to #233.

@jasjuang jasjuang closed this as completed Nov 7, 2019
@syoyo
Copy link
Collaborator

syoyo commented Nov 8, 2019

Looking forward to a solution to #233.

You can contribute to support better double-precision support(and also quadruple-precision floating point if required)

@noma
Copy link
Contributor

noma commented Nov 8, 2019

Taking another look on the CMakeFiles.txtit would probably more modern CMake to replace the global:

add_definitions(-DTINYOBJLOADER_USE_DOUBLE)

By something like this, after the add_library():

if(TINYOBJLOADER_USE_DOUBLE)
    target_compile_definitions(${LIBRARY_NAME} PUBLIC TINYOBJLOADER_USE_DOUBLE)
endif()

Other targets within the project as well in external CMake projects using the ${LIBRARY_NAME} target as a dependency should get the define automatically this way.

@jasjuang
Copy link
Contributor Author

jasjuang commented Nov 8, 2019

@noma thanks for the suggestion, it works, please see PR #238.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants