Add optimized parser with multithreading, SIMD, and custom allocator support#424
Add optimized parser with multithreading, SIMD, and custom allocator support#424
Conversation
…support - Add TINYOBJLOADER_USE_MULTITHREADING compile option for multi-threaded parsing - Add TINYOBJLOADER_USE_SIMD compile option for SIMD-accelerated newline scanning (SSE2, AVX2, NEON) - Add ArenaAllocator class for bulk memory management - Add arena_adapter<T> STL-compatible allocator adapter - Add template data structures (basic_attrib_t, basic_mesh_t, basic_shape_t) with custom allocator support - Add OptLoadConfig struct for threading/SIMD configuration - Add LoadObjOpt API for optimized loading from buffer and file - Implement multi-threaded parsing pipeline (line detection -> parsing -> merge) - Add comprehensive tests for all new features Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
…l_basedir, use nullptr Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new C++11+ “optimized” OBJ loading path to tiny_obj_loader.h, optionally using multithreading and SIMD for line scanning, plus an arena allocator and allocator-aware container types.
Changes:
- Introduces C++11-gated optimized API types (
ArenaAllocator,arena_adapter,basic_*templates) andLoadObjOpt()overloads. - Implements a new parsing pipeline with optional SIMD newline scanning and optional multithreaded parsing/merge.
- Adds unit tests covering
LoadObjOpt(buffer/file), triangulation behavior, and allocator utilities.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| tiny_obj_loader.h | Adds new optimized public API surface and a large in-header implementation (allocator, SIMD scan, multithread parsing, file/buffer loaders). |
| tests/tester.cc | Adds new tests targeting the optimized API and arena allocator utilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tiny_obj_loader.h
Outdated
| command->mtllib_name = p + (token - linebuf); | ||
| command->mtllib_name_len = static_cast<unsigned int>( | ||
| opt_length_until_newline(token, | ||
| p_len - static_cast<size_t>(token - linebuf)) + | ||
| 1); | ||
| command->type = OPT_CMD_MTLLIB; |
There was a problem hiding this comment.
mtllib_name_len is computed as opt_length_until_newline(...) + 1 and then used to construct std::string from the original OBJ buffer. If the mtllib line is the final line without a newline terminator, this can read past the end of the input buffer. Compute lengths without +1 (or explicitly check for an available newline character) to avoid out-of-bounds reads.
There was a problem hiding this comment.
Fixed in 7d46e5e. Removed the +1 from all four opt_length_until_newline results (mtllib_name_len, material_name_len, group_name_len, object_name_len) to prevent out-of-bounds reads when the directive is on the last line without a trailing newline. Also guarded the C++11 tests behind #if __cplusplus >= 201103L to fix the CI build failure under the default -std=c++03.
tiny_obj_loader.h
Outdated
| std::string filepath(filename); | ||
| std::ifstream ifs(filepath, std::ios::binary | std::ios::ate); | ||
| if (!ifs.is_open()) { | ||
| if (err) *err = "Cannot open file: " + filepath; | ||
| return false; | ||
| } | ||
|
|
||
| std::streamsize fsize = ifs.tellg(); | ||
| ifs.seekg(0, std::ios::beg); | ||
|
|
||
| if (fsize <= 0) { | ||
| return true; // empty file | ||
| } | ||
|
|
||
| std::vector<char> buf(static_cast<size_t>(fsize)); | ||
| if (!ifs.read(buf.data(), fsize)) { | ||
| if (err) *err = "Failed to read file: " + filepath; |
There was a problem hiding this comment.
LoadObjOpt(file) always reads the full file into a std::vector. The PR description says the optimized loader is "memory-mapped when possible", but this implementation doesn't use the existing MappedFile (TINYOBJLOADER_USE_MMAP) path. Either implement mmap-backed loading here (to match the description and avoid extra copies for huge meshes) or update the PR description/API docs to reflect the actual behavior.
There was a problem hiding this comment.
Updated in 3038926. Removed the "memory-mapped when possible" claim from the LoadObjOpt(file) API comment since the implementation reads via std::ifstream into a std::vector<char>.
tiny_obj_loader.h
Outdated
| char linebuf[4096]; | ||
| size_t copy_len = (p_len < 4095) ? p_len : 4095; | ||
| std::memcpy(linebuf, p, copy_len); | ||
| linebuf[copy_len] = '\0'; | ||
|
|
||
| const char *token = linebuf; |
There was a problem hiding this comment.
opt_parseLine copies each line into a fixed 4096-byte buffer and truncates longer lines. For OBJ, very long face lines are common; truncation will silently drop vertices/indices and produce corrupted meshes. Consider parsing directly from (p, p_len) without copying, or dynamically sizing the buffer based on p_len (with bounds checks).
| char linebuf[4096]; | |
| size_t copy_len = (p_len < 4095) ? p_len : 4095; | |
| std::memcpy(linebuf, p, copy_len); | |
| linebuf[copy_len] = '\0'; | |
| const char *token = linebuf; | |
| // Use a dynamically sized buffer to avoid truncating long OBJ lines. | |
| std::string linebuf(p, p_len); | |
| linebuf.push_back('\0'); | |
| const char *token = linebuf.c_str(); |
There was a problem hiding this comment.
@copilot don't create bufer for line. just hold start address and its length.
There was a problem hiding this comment.
Fixed in 3038926. Removed the char linebuf[4096] copy entirely. opt_parseLine now works directly with the (p, p_len) pointer and length from the original buffer. Added '\n' to the stop-character sets in opt_until_space and opt_parseRawTriple so tokenization terminates at line boundaries. A sentinel newline is appended once in LoadObjOpt (not per-line) when the buffer lacks a trailing newline. Added test test_loadobjopt_no_trailing_newline to verify.
| template <typename Alloc = std::allocator<char>> | ||
| struct basic_attrib_t { | ||
| using real_alloc = | ||
| typename std::allocator_traits<Alloc>::template rebind_alloc<real_t>; | ||
| using int_alloc = | ||
| typename std::allocator_traits<Alloc>::template rebind_alloc<int>; | ||
| using index_alloc = | ||
| typename std::allocator_traits<Alloc>::template rebind_alloc<index_t>; | ||
|
|
||
| std::vector<real_t, real_alloc> vertices; // xyz | ||
| std::vector<real_t, real_alloc> normals; // xyz | ||
| std::vector<real_t, real_alloc> texcoords; // uv | ||
| std::vector<real_t, real_alloc> colors; // rgb (optional) | ||
| std::vector<index_t, index_alloc> indices; // face indices | ||
| std::vector<int, int_alloc> face_num_verts; // verts per face | ||
| std::vector<int, int_alloc> material_ids; // per-face material | ||
| }; |
There was a problem hiding this comment.
basic_attrib_t/basic_mesh_t/basic_shape_t are templated on an allocator type, but there is no way to pass a stateful allocator instance into the member vectors (they will default-construct Alloc). For arena_adapter this means the vectors will silently fall back to heap allocations unless constructed with an allocator instance. Consider adding constructors that take an Alloc (and wiring it to all member containers, possibly via std::scoped_allocator_adaptor), or document that Alloc must be stateless.
| // ---- Tests for Optimized API (LoadObjOpt) ---- | ||
|
|
||
| void test_loadobjopt_from_buffer() { | ||
| // Simple triangle | ||
| const char *obj_text = |
There was a problem hiding this comment.
These new tests directly reference C++11-only types/APIs (basic_attrib_t, LoadObjOpt, ArenaAllocator), but tests/Makefile defaults to -std=c++03. Without guarding these tests behind the same C++11 check used in the header (or updating the test build flags), the test suite will fail to compile in the default configuration.
tiny_obj_loader.h
Outdated
| command->object_name = p + (token - linebuf); | ||
| command->object_name_len = static_cast<unsigned int>( | ||
| opt_length_until_newline(token, | ||
| p_len - static_cast<size_t>(token - linebuf)) + | ||
| 1); | ||
| command->type = OPT_CMD_O; | ||
| return true; |
There was a problem hiding this comment.
object_name_len is computed as opt_length_until_newline(...) + 1 and then used with a pointer into the original OBJ buffer. If this is the last line without a newline terminator, the length can extend past the provided buffer. Use p_len - offset (no +1) and trim line endings separately to avoid out-of-bounds reads.
tiny_obj_loader.h
Outdated
| if (cmd.type == OPT_CMD_MTLLIB) { | ||
| mtllib_t = t; | ||
| mtllib_i = | ||
| static_cast<int>(thread_commands[static_cast<size_t>(t)].size()); | ||
| } |
There was a problem hiding this comment.
In the multithreaded parse phase, mtllib_t/mtllib_i are written from multiple worker threads without synchronization. This is a data race (UB) and also makes the selected mtllib directive nondeterministic. Record mtllib positions per-thread (or use an atomic + compare-exchange on the earliest global line index) and resolve deterministically after joining.
|
|
||
| // Verify the template compiles and works | ||
| tinyobj::basic_attrib_t<ArenaAlloc> attrib; |
There was a problem hiding this comment.
test_basic_attrib_with_arena doesn't actually use the ArenaAllocator: basic_attrib_t<ArenaAlloc> attrib; default-constructs arena_adapter with a null arena pointer, so allocations fall back to the global heap. To validate arena-backed containers, construct the vectors (or the attrib object) with an allocator instance bound to arena, and assert that allocations come from the arena.
| // Verify the template compiles and works | |
| tinyobj::basic_attrib_t<ArenaAlloc> attrib; | |
| ArenaAlloc alloc(&arena); | |
| // Verify the template compiles and works | |
| tinyobj::basic_attrib_t<ArenaAlloc> attrib(alloc); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tiny_obj_loader.h
Outdated
| idx.texcoord_index = | ||
| opt_fixIndex(vi.texcoord_index, static_cast<int>(tc)); | ||
| idx.normal_index = | ||
| opt_fixIndex(vi.normal_index, static_cast<int>(nc)); |
There was a problem hiding this comment.
opt_index_t uses -1 as the default for missing vt/vn, but -1 is also a valid OBJ relative index meaning “last element”. In OPT_CMD_F merge, opt_fixIndex is applied unconditionally to texcoord_index/normal_index, so missing fields like f 1//3 (or f 1 2 3) are misinterpreted as relative indices and become tc-1/nc-1. This also makes it impossible to correctly represent an explicit -1 index versus an omitted field. To fix this, track presence of vt/vn in opt_parseRawTriple (e.g., boolean flags or a distinct sentinel) and only apply index-fixup when that component was actually present in the face token.
| idx.texcoord_index = | |
| opt_fixIndex(vi.texcoord_index, static_cast<int>(tc)); | |
| idx.normal_index = | |
| opt_fixIndex(vi.normal_index, static_cast<int>(nc)); | |
| if (vi.texcoord_index == -1) { | |
| idx.texcoord_index = -1; | |
| } else { | |
| idx.texcoord_index = | |
| opt_fixIndex(vi.texcoord_index, static_cast<int>(tc)); | |
| } | |
| if (vi.normal_index == -1) { | |
| idx.normal_index = -1; | |
| } else { | |
| idx.normal_index = | |
| opt_fixIndex(vi.normal_index, static_cast<int>(nc)); | |
| } |
tiny_obj_loader.h
Outdated
| /// Scalar fallback newline scanning | ||
| static void scalar_find_line_infos(const char *buf, size_t start, size_t end, | ||
| std::vector<LineInfo> &out) { | ||
| size_t prev = start; | ||
| for (size_t i = start; i < end; i++) { | ||
| if (buf[i] == '\n') { | ||
| size_t line_len = i - prev; | ||
| if (line_len > 0 && buf[prev + line_len - 1] == '\r') line_len--; | ||
| if (line_len > 0) { | ||
| LineInfo info; | ||
| info.pos = prev; | ||
| info.len = line_len; | ||
| out.push_back(info); | ||
| } | ||
| prev = i + 1; | ||
| } | ||
| } | ||
| if (prev < end) { | ||
| size_t line_len = end - prev; | ||
| if (line_len > 0 && buf[prev + line_len - 1] == '\r') line_len--; | ||
| if (line_len > 0) { | ||
| LineInfo info; | ||
| info.pos = prev; | ||
| info.len = line_len; | ||
| out.push_back(info); | ||
| } | ||
| } |
There was a problem hiding this comment.
The line-boundary detection only splits on \n (both SIMD and scalar paths). OBJ files using classic Mac-style \r line endings (or buffers with \r separators) will be treated as a single line, breaking parsing. The legacy StreamReader path treats both \n and \r as line endings; the optimized path should too (handle \r\n, bare \r, and bare \n).
tiny_obj_loader.h
Outdated
| read = 1; | ||
| end_not_reached = (curr != s_end); | ||
| while (end_not_reached && TINYOBJ_OPT_IS_DIGIT(*curr)) { | ||
| double frac_value = 1.0; | ||
| for (int f = 0; f < read; f++) frac_value *= 0.1; | ||
| mantissa += static_cast<int>(*curr - '0') * frac_value; | ||
| read++; |
There was a problem hiding this comment.
The fractional parsing loop in opt_tryParseDouble recomputes frac_value from scratch for every digit via an inner for loop, making decimal parsing O(n^2) in the number of digits. This can dominate runtime for long numeric fields and undermines the “optimized” goal. Consider using the existing tryParseDouble implementation (it uses a LUT / pow) or accumulate a running scale factor (frac *= 0.1) per digit.
| read = 1; | |
| end_not_reached = (curr != s_end); | |
| while (end_not_reached && TINYOBJ_OPT_IS_DIGIT(*curr)) { | |
| double frac_value = 1.0; | |
| for (int f = 0; f < read; f++) frac_value *= 0.1; | |
| mantissa += static_cast<int>(*curr - '0') * frac_value; | |
| read++; | |
| end_not_reached = (curr != s_end); | |
| double frac_scale = 0.1; | |
| while (end_not_reached && TINYOBJ_OPT_IS_DIGIT(*curr)) { | |
| mantissa += static_cast<int>(*curr - '0') * frac_scale; | |
| frac_scale *= 0.1; |
| static inline unsigned int tinyobj_ctz(unsigned int x) { | ||
| #if defined(_MSC_VER) | ||
| unsigned long idx; | ||
| _BitScanForward(&idx, x); | ||
| return static_cast<unsigned int>(idx); | ||
| #else | ||
| return static_cast<unsigned int>(__builtin_ctz(x)); | ||
| #endif |
There was a problem hiding this comment.
MSVC path in tinyobj_ctz calls _BitScanForward but this header doesn’t include <intrin.h>, which is where _BitScanForward is declared. This will fail to compile on MSVC when TINYOBJLOADER_USE_SIMD is enabled. Consider adding #include <intrin.h> under _MSC_VER (and possibly #pragma intrinsic(_BitScanForward) if needed).
tiny_obj_loader.h
Outdated
| size_t idx_start = 0; | ||
| for (size_t fi = 0; fi < face_prev_offset; fi++) | ||
| idx_start += | ||
| static_cast<size_t>(attrib->face_num_verts[fi]); | ||
| size_t idx_end = idx_start; | ||
| for (size_t fi = face_prev_offset; fi < face_count; fi++) | ||
| idx_end += | ||
| static_cast<size_t>(attrib->face_num_verts[fi]); |
There was a problem hiding this comment.
Shape construction recomputes idx_start/idx_end by summing face_num_verts from the beginning for every group/object boundary. With many groups, this becomes O(#faces × #groups). Consider precomputing a prefix-sum array of index offsets (or tracking a running index cursor) so slicing indices/material_ids is O(1) per shape boundary.
tiny_obj_loader.h
Outdated
|
|
||
| if (*curr == '+' || *curr == '-') { | ||
| sign = *curr; | ||
| curr++; | ||
| } else if (!TINYOBJ_OPT_IS_DIGIT(*curr)) { | ||
| return false; | ||
| } | ||
|
|
||
| end_not_reached = (curr != s_end); | ||
| while (end_not_reached && TINYOBJ_OPT_IS_DIGIT(*curr)) { | ||
| mantissa *= 10; | ||
| mantissa += static_cast<int>(*curr - '0'); | ||
| curr++; | ||
| read++; | ||
| end_not_reached = (curr != s_end); | ||
| } | ||
| if (read == 0) return false; |
There was a problem hiding this comment.
opt_tryParseDouble rejects floats that start with a decimal dot (e.g. .7, -.5234), while the existing tryParseDouble explicitly supports that syntax. This will make LoadObjOpt fail to parse valid OBJ numeric fields and can change geometry. Consider reusing the existing tryParseDouble/parseReal logic (or at least match its accepted grammar, including leading .).
| if (*curr == '+' || *curr == '-') { | |
| sign = *curr; | |
| curr++; | |
| } else if (!TINYOBJ_OPT_IS_DIGIT(*curr)) { | |
| return false; | |
| } | |
| end_not_reached = (curr != s_end); | |
| while (end_not_reached && TINYOBJ_OPT_IS_DIGIT(*curr)) { | |
| mantissa *= 10; | |
| mantissa += static_cast<int>(*curr - '0'); | |
| curr++; | |
| read++; | |
| end_not_reached = (curr != s_end); | |
| } | |
| if (read == 0) return false; | |
| bool has_leading_decimal = false; | |
| if (*curr == '+' || *curr == '-') { | |
| sign = *curr; | |
| curr++; | |
| } | |
| if (curr == s_end) return false; | |
| if (*curr == '.') { | |
| // Allow numbers that start with a decimal point, e.g. ".7" or "-.5234". | |
| has_leading_decimal = true; | |
| } else if (!TINYOBJ_OPT_IS_DIGIT(*curr)) { | |
| return false; | |
| } | |
| end_not_reached = (curr != s_end); | |
| if (!has_leading_decimal) { | |
| while (end_not_reached && TINYOBJ_OPT_IS_DIGIT(*curr)) { | |
| mantissa *= 10; | |
| mantissa += static_cast<int>(*curr - '0'); | |
| curr++; | |
| read++; | |
| end_not_reached = (curr != s_end); | |
| } | |
| if (read == 0) return false; | |
| } |
tiny_obj_loader.h
Outdated
| command->type = OPT_CMD_F; | ||
|
|
||
| if (triangulate) { | ||
| opt_index_t i0 = (face_count <= 8) ? face_buf[0] : command->f[0]; | ||
| if (face_count <= 8) { | ||
| for (int k = 2; k < face_count; k++) { | ||
| command->f.push_back(i0); | ||
| command->f.push_back(face_buf[k - 1]); | ||
| command->f.push_back(face_buf[k]); | ||
| command->f_num_verts.push_back(3); | ||
| } |
There was a problem hiding this comment.
Face triangulation assumes face_count >= 3 and unconditionally reads face_buf[0]/command->f[0]. For malformed/degenerate f lines with fewer than 3 vertices, this becomes undefined behavior (out-of-bounds/uninitialized read) instead of emitting a warning and skipping like the legacy parser does. Add a validation check for face_count < 3 (and handle face_count < 1 in the non-triangulate path) before accessing the buffers.
| // ---- Tests for Optimized API (LoadObjOpt) ---- | ||
|
|
||
| void test_loadobjopt_from_buffer() { | ||
| // Simple triangle | ||
| const char *obj_text = | ||
| "v 0.0 0.0 0.0\n" | ||
| "v 1.0 0.0 0.0\n" | ||
| "v 0.0 1.0 0.0\n" | ||
| "vn 0.0 0.0 1.0\n" | ||
| "vt 0.0 0.0\n" | ||
| "vt 1.0 0.0\n" | ||
| "vt 0.0 1.0\n" | ||
| "f 1/1/1 2/2/1 3/3/1\n"; | ||
| size_t obj_len = strlen(obj_text); | ||
|
|
||
| tinyobj::basic_attrib_t<> attrib; | ||
| std::vector<tinyobj::basic_shape_t<>> shapes; | ||
| std::vector<tinyobj::material_t> materials; | ||
| std::string warn, err; | ||
|
|
||
| tinyobj::OptLoadConfig config; | ||
| config.num_threads = 1; // force single-threaded for determinism | ||
| config.triangulate = true; | ||
|
|
||
| bool ret = tinyobj::LoadObjOpt(&attrib, &shapes, &materials, &warn, &err, | ||
| obj_text, obj_len, config); | ||
| if (!err.empty()) std::cerr << "ERR: " << err << "\n"; |
There was a problem hiding this comment.
These new tests (and the optimized API types they reference) require C++11 (basic_attrib_t, OptLoadConfig, ArenaAllocator, uintptr_t, etc.), but tests/Makefile currently defaults to -std=c++03. As-is, the test suite will fail to compile in the default configuration. Consider guarding the new test cases with a C++11 preprocessor check (and/or updating the test build flags in this PR) so C++03 builds remain green.
tiny_obj_loader.h
Outdated
| (mat_name.back() == '\r' || mat_name.back() == '\n')) | ||
| mat_name.pop_back(); | ||
| auto it = material_map.find(mat_name); | ||
| int mat_id = (it != material_map.end()) ? it->second : -2; |
There was a problem hiding this comment.
When a usemtl name isn’t found in material_map, this sets mat_id to -2. The legacy API uses -1 for “no/unknown material” and also emits a warning. Using a different sentinel can break downstream code that assumes -1 semantics. Consider using -1 here and (optionally) appending a warning like the legacy path does.
| int mat_id = (it != material_map.end()) ? it->second : -2; | |
| int mat_id = (it != material_map.end()) ? it->second : -1; |
tiny_obj_loader.h
Outdated
| /// Optimized loader — load from a file (memory-mapped when possible). | ||
| bool LoadObjOpt(basic_attrib_t<> *attrib, | ||
| std::vector<basic_shape_t<>> *shapes, | ||
| std::vector<material_t> *materials, | ||
| std::string *warn, std::string *err, | ||
| const char *filename, | ||
| const char *mtl_basedir = nullptr, | ||
| const OptLoadConfig &config = OptLoadConfig()); |
There was a problem hiding this comment.
The API comment says the file overload is "memory-mapped when possible", but the implementation always reads via std::ifstream into a std::vector<char> (no mmap path). Either implement mmap support here (possibly reusing the existing MappedFile infrastructure) or adjust the comment/PR description to avoid overstating behavior.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Wrap optimized API tests (LoadObjOpt, ArenaAllocator, etc.) in #if __cplusplus >= 201103L so the test suite compiles under -std=c++03 (the CI default). - Remove the unconditional +1 from mtllib_name_len, material_name_len, group_name_len, and object_name_len computations in opt_parseLine to prevent out-of-bounds reads when the line is the last in the buffer with no trailing newline. Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
…ing, update API comment - opt_parseLine: remove fixed 4096-byte buffer copy, parse directly from (p, p_len) without per-line allocation. A sentinel newline is appended in LoadObjOpt when the buffer lacks a trailing newline. - opt_tryParseDouble: accept leading decimal dots (e.g. ".7", "-.5234") to match the existing tryParseDouble behavior. Also fix O(n²) fractional loop by accumulating a running scale factor. - opt_until_space / opt_parseRawTriple: add '\n' to stop-character set so tokenization terminates correctly without null-terminated copies. - LoadObjOpt(file) API comment: remove "memory-mapped when possible" claim since the implementation reads via std::ifstream. - Add tests for leading decimal dot parsing and no-trailing-newline buffers. Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Resolve merge conflicts keeping both sides, remove C++11 preprocessor guards (now enforced via #error), replace TINYOBJ_OVERRIDE macro with direct `override` keyword, and add #ifndef guards for macro redefinition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@copilot Ultra super titanium review |
…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>
…ruct formatting Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Completed comprehensive review in 2a57c8b and 3ab01d6. All remaining feedback has been addressed:
14 tests total, all passing. CodeQL clean. |
|
@copilot deeper review |
Previously, both opt_parseLine and the fast path stored extra[0] (the weight) as color R when 3+ extra components were present. For v x y z w r g b (7 total), color was incorrectly (w,r,g) instead of (r,g,b), losing the blue channel entirely. Now correctly handles: - v x y z (3) — no weight, no color - v x y z w (4) — weight=w, no color - v x y z r g b (6) — weight=r (legacy compat), color=(r,g,b) - v x y z w r g b (7) — weight=w, color=(r,g,b) Fixed in both opt_parseLine (slow path) and opt_parseLineToThreadData (fast path). Added test verifying 6 and 7 component parsing for both LoadObjOpt and LoadObjOptTyped. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@copilot final ultra super think think deep review |
…t fixes for DISABLE_FAST_FLOAT Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Completed in dc2688f, 12de488, and 19f8f03. Deep review findings and fixes:
All 200 tests pass in default, SIMD, MT, and DISABLE_FAST_FLOAT modes. CodeQL clean. |
The exponent accumulation loop `exponent *= 10` could overflow int for inputs like "1.0e9999999999", which is undefined behavior in C++. Clamped to 0x7FFFFFF (~134M) before the multiply — values above 308 already exceed double range, so further digits are irrelevant. Only affects TINYOBJLOADER_DISABLE_FAST_FLOAT builds (the hand-written float parser fallback). Verified with UBSan fuzzing (15K iterations). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@copilot final ultra super think think deep review, again |
…te overflow guard Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
…condition Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Completed in 44b6270 and 095bbd5. Comprehensive review of the entire optimized parser (~4,000 lines). Found and fixed 2 issues:
Full review confirmed correctness of:
All 200+ tests pass in default, SIMD, MT, and DISABLE_FAST_FLOAT modes. CodeQL clean. |
By default, the library no longer throws C++ exceptions: - ArenaAllocator::allocate returns nullptr on overflow instead of throwing std::bad_alloc - ArenaAllocator::new_block uses new(std::nothrow) and returns nullptr on allocation failure - arena_adapter::allocate uses operator new(std::nothrow) for the non-arena fallback path - TypedArray::allocate returns bool (false on failure) and handles nullptr from the arena gracefully Define TINYOBJLOADER_ENABLE_EXCEPTION before including the header to restore the previous throwing behavior. This enables use in -fno-exceptions environments (embedded, game engines, etc.) without build errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@copilot ultra super finest mitochondria-level detailed review with 1000 years effort. |
…de removal, arena_adapter overflow test Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Completed in 4bb4e2b and 614cca0. Comprehensive mitochondria-level review of the entire optimized parser (~4,000 lines). Found and fixed 4 issues:
Full review confirmed correctness of:
New test |
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.