GSOC 26: render multi-material models per part#8955
Conversation
Nixxx19
changed the title
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Looking good! I left a few comments. One thing that would be good to clarify is what the valid states of a geometry are (should they always have parts?) so that that can be an invariant in the code and simplifies implementation.
thanks for the review! on the invariant, yeah a geometry should always have at least one part, the constructor guarantees it (parts = [this]) and loaders replace it. i'll lean into that so the renderer just loops parts with no fallback, like you said, addressing the rest now |
|
heads up, the failing check is just flaky ci, not anything in this pr. |
| const state = part.partState; | ||
| const hasMaterial = state && ( | ||
| state.fill || state.texture || state.ambientColor || | ||
| state.specularColor || state.shininess != null |
There was a problem hiding this comment.
Heads up, != will consider 0 to be equivalent to null as both are falsy. Do you want !== here? (Also worth double checking that one of these could have valid falsy values -- to confirm, are fill and ambientColor p5.Color values or arrays?)
There was a problem hiding this comment.
these are all plain arrays, not p5.Color, fill is [r,g,b,a], ambient/specular are [r,g,b], texture's a p5.Image. shininess is the only number, and 0 != null is already true so a 0 still counts as set. made them all != null anyway so it stays consistent
| if (material.diffuseColor) { | ||
| // carry the mtl transparency (d / Tr) in the fill's alpha. opaque materials | ||
| // stay rgb so nothing changes for them. | ||
| state.fill = material.opacity != null && material.opacity < 1 |
There was a problem hiding this comment.
It might simplify things further to have the fill always be in the same format. So you could do [...material.diffuseColor, material.opacity ?? 255] assuming a 0-255 range. It's also not obvious what the data type and range should be for each of these so it's maybe worth adding in a comment where part state is first defined what the type and range of each field is, and then ensuring that they are always that type (e.g. [number, number, number, number] | null where the numbers are 0-255)
There was a problem hiding this comment.
agreed, fill's always [r,g,b,a] now. one thing, the range is 0..1 not 0..255 (curFillColor's 0..1, and Kd/d parse straight to floats), so it came out [...diffuseColor, opacity ?? 1]. also documented the type/range of each field on createPartState, and dropped the alpha padding in _applyPartState + hasFillTransparency now that fill's 4 wide
Nixxx19
force-pushed
the
renderer-per-part
branch
from
59b2cce to
e861e3c
Compare
2 participants
part of my google summer of code 2026 project, "full texture support for .mtl files in p5.js". works toward #6924.
this is phase 3: the renderer side, building on the data + parser foundation in #8879. multi-material models now actually draw, one part per material, each with its own look. public api is unchanged, you still just call loadModel() and model().
project phases for context:
what's in here:
tests:
stacked on #8879, so it should go in after that one merges.