Skip to content

fix: gracefully handle mixed-material OBJ models instead of crashing#8666

Open
Nixxx19 wants to merge 1 commit intoprocessing:dev-2.0from
Nixxx19:fix/mixed-material-crash-loading
Open

fix: gracefully handle mixed-material OBJ models instead of crashing#8666
Nixxx19 wants to merge 1 commit intoprocessing:dev-2.0from
Nixxx19:fix/mixed-material-crash-loading

Conversation

@Nixxx19
Copy link

@Nixxx19 Nixxx19 commented Mar 22, 2026

Summary

Fixes a crash in parseObj() (loading.js:655-658) that broke loading of real-world OBJ files exported from Blender, Sketchfab, and similar tools.

Root cause

The check used to detect inconsistent vertex coloring was:

// BEFORE (broken)
if (hasColoredVertices === hasColorlessVertices) {
  throw new Error('Model coloring is inconsistent...');
}

The === comparison fires in two completely different situations:

State Meaning Should crash? Did crash?
both false Model has no faces at all ✅ Bug
both true Some faces have material colors, some don't ✅ Bug

Many standard OBJ exports have only some mesh groups with an explicit usemtl entry — this is valid OBJ syntax and common in practice.

Fix

Changed the condition to only fire when both flags are true (the genuinely mixed case), and replaced the thrown error with a console.warn + vertex color strip so the model still loads:

// AFTER (fixed)
if (hasColoredVertices && hasColorlessVertices) {
  console.warn('p5.js: This OBJ model has mixed material coloring...');
  model.vertexColors = [];
}

Accessibility impact

Beginners loading common 3D assets from Sketchfab or Blender would get an opaque crash message with no path forward. This fix makes those models load and render with the default fill color, keeping the loadModel() experience frustration-free.

Test changes

The existing test 'inconsistent vertex coloring throws error' is updated to assert that a mixed-material model loads successfully and returns a Geometry with vertexColors.length === 0.

All 13 loadModel unit tests pass ✅

Test plan

  • npx vitest run test/unit/io/loadModel.js — all 13 tests pass
  • Mixed-material model (eg1.obj) now loads successfully and emits a console.warn
  • Fully-colored models and no-material models are unaffected
  • No changes to public API

@welcome
Copy link

welcome bot commented Mar 22, 2026

🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors!
🤔 Please ensure that your PR links to an issue, which has been approved for work by a maintainer; otherwise, there might already be someone working on it, or still ongoing discussion about implementation. You are welcome to join the discussion in an Issue if you're not sure!
🌸 Once your PR is merged, be sure to add yourself to the list of contributors on the readme page !

Thank You!

Previously, `parseObj()` used `hasColoredVertices === hasColorlessVertices`
to detect inconsistent vertex coloring, but this condition is true in two
very different cases:

1. Both flags `false`  → model has no faces at all (should never crash)
2. Both flags `true`   → model has some faces with material colors and
                         some without (the genuine "mixed" case)

Real-world OBJ exports from Blender, Sketchfab, and other tools commonly
produce files where only some mesh groups have an explicit MTL material
assignment. The previous `throw` caused a hard crash for any such model,
which is a significant barrier for beginners trying to use pre-made 3D
assets in p5.js.

This commit changes the check to `hasColoredVertices && hasColorlessVertices`
(only the genuinely inconsistent case) and replaces the thrown error with a
`console.warn` + `model.vertexColors = []` reset, so the model still loads
and renders using the default fill color.

The existing test that expected a throw is updated to assert that the model
loads successfully with an empty vertexColors array.
@Nixxx19 Nixxx19 force-pushed the fix/mixed-material-crash-loading branch from b9edf9e to 82122e1 Compare March 22, 2026 01:54
Nixxx19 added a commit to Nixxx19/p5.js that referenced this pull request Mar 22, 2026
- change pr processing#8666 from "merged" to "open" (accurate status)
- remove "this is itself a deliverable" over-explanation
- remove "this is my honest answer on it" defensive phrasing
- rewrite section 11 opener: drop "not because i am unsure" framing
- remove draft footer from end of file

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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

Successfully merging this pull request may close these issues.

1 participant