Fix OBJ "g" statement dropping single/double group names#132
Conversation
ObjImporter treated 'g' with two or fewer names (argCount <= 2) as "no group":
it filed the geometry under the default group and discarded the names. That
swallows the two most common forms - 'g name' and 'g name1 name2' - and only
honored a 'g' with three or more names, which is backwards. The comment right
below ("Each token is a name") shows the intent: each token after the keyword
is a group name. Changed the guard to argCount < 1, so only a bare 'g' selects
the default group and any named group is recorded.
Also expose the group map via ObjGeometryStore.getGroupMap(), matching the
existing getMaterialMap(). The map was populated but write-only - there was no
way to read back which spatial belongs to which group, so even a correctly
parsed group name went nowhere. The getter makes the parsed groups usable and
lets the fix be tested behaviorally.
Red->green test covers the single-name, two-name, and bare-'g' (default) forms.
Copyright headers on changed files bumped to 2008-2026.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 10 minutes and 3 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesOBJ Group Parsing Fix and Multi-Spatial Group Storage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ardor3d-extras/src/main/java/com/ardor3d/extension/model/obj/ObjGeometryStore.java`:
- Around line 352-357: The getGroupMap() method returns the internal _groupMap
directly, which allows callers to modify it and corrupt the store's internal
state. To fix this, wrap the return value using Collections.unmodifiableMap() so
that the returned map cannot be modified by external callers. Update the return
statement in the getGroupMap() method to return
Collections.unmodifiableMap(_groupMap) instead of returning _groupMap directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47b38d5a-1469-478e-bb8b-4b47c310a7b3
📒 Files selected for processing (3)
ardor3d-extras/src/main/java/com/ardor3d/extension/model/obj/ObjGeometryStore.javaardor3d-extras/src/main/java/com/ardor3d/extension/model/obj/ObjImporter.javaardor3d-extras/src/test/java/com/ardor3d/extension/model/obj/TestObjImporterGroups.java
Address CodeRabbit review on #132: getGroupMap() is new public API, so wrap the returned map in Collections.unmodifiableMap so callers can't mutate the store's internal _groupMap. (Scoped to the getter added here; the pre-existing getMaterialMap/getMaterialLibrary getters are left as-is in this focused PR.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the ObjImporter "g"-block conflict: keep the argCount < 1 group fix and drop the stale commented-out throw lines (this branch's intent), on top of the import-hardening changes that landed on master.
Local CodeRabbit review caught that getGroupMap()'s backing _groupMap was Map<String, Spatial> - one spatial per group name. But commitObjects() fires on every "usemtl"/"o" change within a group, so a group that spans multiple materials or objects commits several spatials that all map to the same name, and each put() overwrote the last - so getGroupMap() returned only the final spatial and silently dropped the rest. Multi-material groups are common in real OBJ files, so this made the new accessor lossy. Changed _groupMap to Map<String, List<Spatial>> and accumulate via computeIfAbsent (still returned as an unmodifiable view). Added a test for a group spanning two "o" objects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up from the local CodeRabbit review: the outer map was unmodifiable but the per-group Spatial lists were not, so getGroupMap().get(name).clear() could still corrupt internal state. Return a view whose lists are each wrapped in Collections.unmodifiableList. (Called rarely - a post-load result read - so the per-call view build is negligible.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
ObjImportertreated agstatement with two or fewer names (argCount <= 2) as "no group" — it filed the geometry under the default group and threw the names away. That silently swallows the two most common forms,g nameandg name1 name2, and only honored agwith three or more names, which is backwards. The comment right below (// Each token is a name) confirms the intent: every token after the keyword is a group name.argCount < 1, so only a baregselects the default group and any named group is recorded. Dropped the stale commented-outthrow new Error(...)lines.ObjGeometryStore.getGroupMap()(mirrors the existinggetMaterialMap()). The group map was populated but write-only — there was no accessor, so even a correctly-parsed group name went nowhere observable. The getter makes parsed groups usable by callers and lets the fix be tested behaviorally.No regression risk: the map had no readers, so changing what it's keyed by is invisible to existing code; the getter is purely additive.
Testing
New red→green test (
TestObjImporterGroups) covering the single-name, two-name, and bare-g(default) forms. Fullardor3d-extrassuite green; whole-projectcompileJavaclean. Copyright headers on changed files bumped to 2008-2026.Independent of #131 (different lines / new method), so the two can merge in either order.
🤖 Generated with Claude Code
Summary by CodeRabbit
g(group) handling so only a truly emptygselects the default group;glines with 1–2 names are now recorded correctly.gline and group continuity across multiple objects.