Skip to content

Commit 2a57c8b

Browse files
Copilotsyoyo
Copilot
and
syoyo
committed
Comprehensive review: fix data races, face validation, line endings, index sentinels, shape perf
- Fix multi-threaded mtllib_t/mtllib_i data race: use per-thread tracking and resolve deterministically (earliest line wins) after join - Fix opt_index_t sentinel: use INT_MIN for "not present" to distinguish from OBJ relative index -1 ("last element") - Fix OPT_CMD_F merge: skip opt_fixIndex for missing vt/vn components (use -1 "not present" instead of misinterpreting as relative index) - Fix usemtl unknown material sentinel: use -1 instead of -2 to match legacy API semantics - Fix face triangulation: validate face_count >= 3 before accessing buffers (skip degenerate faces matching legacy parser behavior) - Fix scalar_find_line_infos: handle bare \r line endings (old Mac style) in addition to \n and \r\n - Fix MSVC SIMD: add #include <intrin.h> for _BitScanForward - Fix OptLoadConfig::verbose doc: mark as reserved/no-op - Fix shape construction: use prefix-sum for O(1) index slicing instead of O(#faces × #groups) recomputation - Fix LoadObjOpt(file) material fallback: remove inconsistent post-parse material loading that populated materials without recomputing material_ids - Add 3 new tests: face_missing_vt_vn, bare_cr_line_endings, degenerate_face Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
1 parent c498e9a commit 2a57c8b

2 files changed

Lines changed: 205 additions & 85 deletions

File tree

‎tests/tester.cc‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3578,6 +3578,108 @@ void test_loadobjopt_no_trailing_newline() {
35783578
TEST_CHECK(attrib.indices.size() == 3); // 3 face indices
35793579
}
35803580

3581+
void test_loadobjopt_face_missing_vt_vn() {
3582+
// Test that missing vt/vn in face lines are correctly handled as -1
3583+
// (not misinterpreted as relative index -1).
3584+
const char *obj_text =
3585+
"v 0.0 0.0 0.0\n"
3586+
"v 1.0 0.0 0.0\n"
3587+
"v 0.0 1.0 0.0\n"
3588+
"vn 0.0 0.0 1.0\n"
3589+
"f 1//1 2//1 3//1\n"; // vertex//normal (no texcoord)
3590+
size_t obj_len = strlen(obj_text);
3591+
3592+
tinyobj::basic_attrib_t<> attrib;
3593+
std::vector<tinyobj::basic_shape_t<>> shapes;
3594+
std::vector<tinyobj::material_t> materials;
3595+
std::string warn, err;
3596+
3597+
tinyobj::OptLoadConfig config;
3598+
config.triangulate = true;
3599+
3600+
bool ret = tinyobj::LoadObjOpt(&attrib, &shapes, &materials, &warn, &err,
3601+
obj_text, obj_len, config);
3602+
TEST_CHECK(ret == true);
3603+
TEST_CHECK(attrib.indices.size() == 3);
3604+
3605+
// texcoord_index should be -1 (not present), not a fixed-up relative index
3606+
for (size_t i = 0; i < attrib.indices.size(); i++) {
3607+
TEST_CHECK(attrib.indices[i].texcoord_index == -1);
3608+
TEST_CHECK(attrib.indices[i].normal_index == 0); // mapped from 1-based
3609+
}
3610+
3611+
// Also test "f 1 2 3" (vertex-only, no texcoord, no normal)
3612+
const char *obj_text2 =
3613+
"v 0.0 0.0 0.0\n"
3614+
"v 1.0 0.0 0.0\n"
3615+
"v 0.0 1.0 0.0\n"
3616+
"f 1 2 3\n";
3617+
size_t obj_len2 = strlen(obj_text2);
3618+
3619+
tinyobj::basic_attrib_t<> attrib2;
3620+
std::vector<tinyobj::basic_shape_t<>> shapes2;
3621+
std::vector<tinyobj::material_t> materials2;
3622+
std::string warn2, err2;
3623+
3624+
ret = tinyobj::LoadObjOpt(&attrib2, &shapes2, &materials2, &warn2, &err2,
3625+
obj_text2, obj_len2, config);
3626+
TEST_CHECK(ret == true);
3627+
TEST_CHECK(attrib2.indices.size() == 3);
3628+
for (size_t i = 0; i < attrib2.indices.size(); i++) {
3629+
TEST_CHECK(attrib2.indices[i].texcoord_index == -1);
3630+
TEST_CHECK(attrib2.indices[i].normal_index == -1);
3631+
}
3632+
}
3633+
3634+
void test_loadobjopt_bare_cr_line_endings() {
3635+
// Test OBJ buffer with bare \r line endings (old Mac style)
3636+
const char *obj_text =
3637+
"v 0.0 0.0 0.0\r"
3638+
"v 1.0 0.0 0.0\r"
3639+
"v 0.0 1.0 0.0\r"
3640+
"f 1 2 3\r";
3641+
size_t obj_len = strlen(obj_text);
3642+
3643+
tinyobj::basic_attrib_t<> attrib;
3644+
std::vector<tinyobj::basic_shape_t<>> shapes;
3645+
std::vector<tinyobj::material_t> materials;
3646+
std::string warn, err;
3647+
3648+
tinyobj::OptLoadConfig config;
3649+
config.triangulate = true;
3650+
3651+
bool ret = tinyobj::LoadObjOpt(&attrib, &shapes, &materials, &warn, &err,
3652+
obj_text, obj_len, config);
3653+
TEST_CHECK(ret == true);
3654+
TEST_CHECK(attrib.vertices.size() == 9); // 3 vertices * 3 coords
3655+
TEST_CHECK(attrib.indices.size() == 3); // 3 face indices
3656+
}
3657+
3658+
void test_loadobjopt_degenerate_face() {
3659+
// Test that faces with fewer than 3 vertices are skipped
3660+
const char *obj_text =
3661+
"v 0.0 0.0 0.0\n"
3662+
"v 1.0 0.0 0.0\n"
3663+
"v 0.0 1.0 0.0\n"
3664+
"f 1 2\n" // degenerate (2 vertices) — should be skipped
3665+
"f 1 2 3\n"; // valid triangle
3666+
size_t obj_len = strlen(obj_text);
3667+
3668+
tinyobj::basic_attrib_t<> attrib;
3669+
std::vector<tinyobj::basic_shape_t<>> shapes;
3670+
std::vector<tinyobj::material_t> materials;
3671+
std::string warn, err;
3672+
3673+
tinyobj::OptLoadConfig config;
3674+
config.triangulate = true;
3675+
3676+
bool ret = tinyobj::LoadObjOpt(&attrib, &shapes, &materials, &warn, &err,
3677+
obj_text, obj_len, config);
3678+
TEST_CHECK(ret == true);
3679+
TEST_CHECK(attrib.indices.size() == 3); // only the valid triangle
3680+
TEST_CHECK(attrib.face_num_verts.size() == 1); // 1 face
3681+
}
3682+
35813683
void test_arena_allocator() {
35823684
tinyobj::ArenaAllocator arena(4096);
35833685

@@ -3737,6 +3839,9 @@ TEST_LIST = {
37373839
{"test_loadobjopt_empty_buffer", test_loadobjopt_empty_buffer},
37383840
{"test_loadobjopt_leading_decimal_dot", test_loadobjopt_leading_decimal_dot},
37393841
{"test_loadobjopt_no_trailing_newline", test_loadobjopt_no_trailing_newline},
3842+
{"test_loadobjopt_face_missing_vt_vn", test_loadobjopt_face_missing_vt_vn},
3843+
{"test_loadobjopt_bare_cr_line_endings", test_loadobjopt_bare_cr_line_endings},
3844+
{"test_loadobjopt_degenerate_face", test_loadobjopt_degenerate_face},
37403845
{"test_arena_allocator", test_arena_allocator},
37413846
{"test_arena_adapter_with_vector", test_arena_adapter_with_vector},
37423847
{"test_basic_attrib_with_arena", test_basic_attrib_with_arena},

0 commit comments

Comments
 (0)