feat(ldraw): add Fabric material variant#66
Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-class support for LDraw’s FABRIC color material by introducing a new Material::Fabric variant and recognizing the FABRIC keyword during LDConfig.ldr color-definition parsing.
Changes:
- Added
Fabricto theMaterialenum. - Updated
parse_color_definitionsto map theFABRICkeyword toMaterial::Fabric.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ldraw/src/color.rs |
Extends the public Material enum with a Fabric variant to represent the LDraw FABRIC material. |
ldraw/src/parser.rs |
Recognizes FABRIC in !COLOUR definitions and assigns Material::Fabric during parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "FABRIC" => { | ||
| material = Material::Fabric; | ||
| } | ||
| "MATERIAL" => { | ||
| material = Material::Custom(parse_customized_material(&mut it)?); | ||
| } |
There was a problem hiding this comment.
Good call — addressed in 637e821. Added a Fabric (code 10) row to COLOR_DEFINITIONS and a matching expected Color with material: Material::Fabric. Note: the top-level FABRIC keyword handled in parse_color_definitions is a separate code path from the MATERIAL FABRIC CANVAS handling in parse_customized_material (introduced by the rust-2024 commit), so this test guards only the new branch.
| pub enum Material { | ||
| Plastic, | ||
| Chrome, | ||
| Pearlescent, | ||
| Rubber, | ||
| MatteMetallic, | ||
| Metal, | ||
| Fabric, | ||
| Custom(CustomizedMaterial), | ||
| } |
There was a problem hiding this comment.
Fair point — addressed in 637e821 by marking Material as #[non_exhaustive]. The enum has been accumulating variants (Glow, Metal, MatteMetallic, now Fabric) and will likely keep doing so as more LDraw materials get covered; #[non_exhaustive] lets future additions avoid the same downstream breakage this PR causes today.
Two follow-ups from review on segfault87#66: - Mark `Material` as `#[non_exhaustive]`. The enum has accumulated variants over time (Glow, Metal, MatteMetallic, now Fabric); flagging it `non_exhaustive` lets future additions avoid the same downstream breakage this PR causes for exhaustive `match`-ers. - Extend `test_parse_color_definition` with a FABRIC color (code 10) so the new top-level `FABRIC` parse branch in `parse_color_definitions` is exercised. (The existing `parse_customized_material` FABRIC arm, introduced by the rust-2024 commit, handles `MATERIAL FABRIC CANVAS` and is on a separate code path.) Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Summary
Adds
Material::Fabricand parses theFABRICmaterial keyword inLDConfig.ldr. LDraw's color spec supports aFABRICmaterial (used for capes, flags, and other soft-goods items); without this variant, color definitions that declareFABRICfall back to default-material handling and lose their material classification.Change
ldraw/src/color.rs: addFabricvariant toMaterialldraw/src/parser.rs: recognize theFABRICkeyword inparse_color_definitionsand assignMaterial::FabricTest plan
cargo check -p ldrawpassesLDConfig.ldrthat contains aFABRIC-material color and confirm the resultingMaterial::Fabric