glTF import mode is ignored so TRIANGLE_STRIP/TRIANGLE_FAN primitives remain mis-imported #841

New issue

Open

opened 2026-09-30 01:25:23 +00:00 by SakulFlee · 0 comments

SakulFlee commented

2026-09-30 01:25:23 +00:00

Owner

Summary

parse_models never reads Primitive::mode(). Every primitive is imported as
if it were a glTF TRIANGLES primitive, so TRIANGLE_STRIP (5) and
TRIANGLE_FAN (6) assets import as garbage geometry instead of failing loudly.

Where the assumption lives

Crates/orbital_importer_gltf/src/gltf/mod.rs, in parse_models
(line numbers as of main @ 5bf04032):

mod.rs:902, mod.rs:921 — the index accessor is read, but mode is
never queried. Neither Primitive::mode() nor gltf::mesh::Mode is
referenced anywhere in the crate.

mod.rs:1072-1081 — the winding flip assumes a triangle list
unconditionally:

// Flip the winding order of indices to account for coordinate system handedness
let mut indices_flipped = Vec::new();
for i in (0..indices_vec.len()).step_by(3) {
    if i + 2 < indices_vec.len() {
        indices_flipped.push(indices_vec[i]);
        indices_flipped.push(indices_vec[i + 2]);
        indices_flipped.push(indices_vec[i + 1]);
    }
}

For a strip or a fan, i/i+1/i+2 are not the vertices of a triangle.

mod.rs:926 onward — the same step_by(3) assumption appears in the
normal calculation, so computed normals are already wrong for strip/fan
before the flip even runs.

The flip also silently discards a trailing partial triangle
(i + 2 < indices_vec.len()), with no warning.

This is not just "read one more field"

orbital_mesh::MeshDescriptor is { vertices, indices } with no topology, and
orbital_material_shader::MaterialShaderDescriptor::primitive_topology is a
per-material field defaulting to PrimitiveTopology::TriangleList. The pipeline
is triangle-list only, so the work splits into two groups:

Expandable into a triangle list, no pipeline change:

mode	value	work
`TRIANGLES`	4	already correct (spec default)
`TRIANGLE_STRIP`	5	expand to N-2 triangles; odd-numbered triangles have reversed winding by definition
`TRIANGLE_FAN`	6	expand to N-2 triangles around vertex 0

Not representable without plumbing a topology through MeshDescriptor and
the render pipeline: POINTS (0), LINES (1), LINE_LOOP (2),
LINE_STRIP (3).

Those two groups are worth separating: the first is a contained importer fix,
the second is a cross-crate change.

⚠️ Numbering trap: the glTF spec numbers are 0–6, but gltf_json's Rust
Mode enum starts at Points = 1. Its discriminants are shifted by one from
the spec values that appear in JSON files. Don't compare raw integers, and
don't trust as u32 on a Mode.

Notes for whoever picks this up

The expansion has to happen before the winding flip, or be folded into
it. The flip's parity interacts with the strip winding rule, so doing it as
two independent passes is an easy way to produce subtly inside-out geometry
on every other triangle.
An unindexed strip or fan is already handled correctly by #840: glTF
defines an unindexed primitive as implicitly indexed 0..N, which is exactly
the vertex order a strip and a fan are defined over. The synthesized 0..N
list is the right starting point — what's missing is only the expansion.
#840 does not make strip/fan any worse; they were already mis-imported.

Suggested test

Follow the inline-.gltf pattern in
Crates/orbital_importer_gltf/tests/import_assets.rs, which builds documents
from serde_json::json! against a temp FileManager. A 4-vertex
"mode": 5 strip should yield 6 indices; a 5-vertex "mode": 6 fan should
yield 9.

Two shapes worth covering:

a strip with an odd triangle count, to pin the winding-parity rule rather
than just the vertex count;
a strip/fan with no NORMAL attribute, so the asserted normals can only
come from the expanded indices reaching the triangle-geometry path.

Open questions

How common are strip/fan assets in the wild? glTF exporters default to
TRIANGLES, so this may be a long tail — and some pipelines run assets
through meshopt, which normalizes to triangles anyway. Worth measuring
against real assets before investing. If the answer is "rare and handled
upstream", documenting the limitation may be the better outcome than a fix.
For the non-triangle modes, should the importer warn-and-skip, or should the
topology actually be plumbed through?
Does the winding flip's silent truncation of a partial triangle deserve its
own issue? It is orthogonal to mode and affects indexed TRIANGLES too.

## Summary `parse_models` never reads `Primitive::mode()`. Every primitive is imported as if it were a glTF `TRIANGLES` primitive, so `TRIANGLE_STRIP` (5) and `TRIANGLE_FAN` (6) assets import as garbage geometry instead of failing loudly. ## Where the assumption lives `Crates/orbital_importer_gltf/src/gltf/mod.rs`, in `parse_models` (line numbers as of `main` @ `5bf04032`): - **`mod.rs:902`, `mod.rs:921`** — the index accessor is read, but `mode` is never queried. Neither `Primitive::mode()` nor `gltf::mesh::Mode` is referenced anywhere in the crate. - **`mod.rs:1072-1081`** — the winding flip assumes a triangle list unconditionally: ```rust // Flip the winding order of indices to account for coordinate system handedness let mut indices_flipped = Vec::new(); for i in (0..indices_vec.len()).step_by(3) { if i + 2 < indices_vec.len() { indices_flipped.push(indices_vec[i]); indices_flipped.push(indices_vec[i + 2]); indices_flipped.push(indices_vec[i + 1]); } } ``` For a strip or a fan, `i`/`i+1`/`i+2` are not the vertices of a triangle. - **`mod.rs:926` onward** — the same `step_by(3)` assumption appears in the normal calculation, so computed normals are already wrong for strip/fan before the flip even runs. The flip also silently discards a trailing partial triangle (`i + 2 < indices_vec.len()`), with no warning. ## This is not just "read one more field" `orbital_mesh::MeshDescriptor` is `{ vertices, indices }` with no topology, and `orbital_material_shader::MaterialShaderDescriptor::primitive_topology` is a per-material field defaulting to `PrimitiveTopology::TriangleList`. The pipeline is triangle-list only, so the work splits into two groups: **Expandable into a triangle list, no pipeline change:** | mode | value | work | |---|---|---| | `TRIANGLES` | 4 | already correct (spec default) | | `TRIANGLE_STRIP` | 5 | expand to N-2 triangles; odd-numbered triangles have reversed winding *by definition* | | `TRIANGLE_FAN` | 6 | expand to N-2 triangles around vertex 0 | **Not representable without plumbing a topology through `MeshDescriptor` and the render pipeline:** `POINTS` (0), `LINES` (1), `LINE_LOOP` (2), `LINE_STRIP` (3). Those two groups are worth separating: the first is a contained importer fix, the second is a cross-crate change. ⚠️ **Numbering trap:** the glTF *spec* numbers are 0–6, but `gltf_json`'s Rust `Mode` enum starts at `Points = 1`. Its discriminants are shifted by one from the spec values that appear in JSON files. Don't compare raw integers, and don't trust `as u32` on a `Mode`. ## Notes for whoever picks this up - The expansion has to happen **before** the winding flip, or be folded into it. The flip's parity interacts with the strip winding rule, so doing it as two independent passes is an easy way to produce subtly inside-out geometry on every other triangle. - An **unindexed** strip or fan is already handled correctly by #840: glTF defines an unindexed primitive as implicitly indexed `0..N`, which is exactly the vertex order a strip and a fan are defined over. The synthesized `0..N` list is the right starting point — what's missing is only the expansion. - #840 does not make strip/fan any worse; they were already mis-imported. ## Suggested test Follow the inline-`.gltf` pattern in `Crates/orbital_importer_gltf/tests/import_assets.rs`, which builds documents from `serde_json::json!` against a temp `FileManager`. A 4-vertex `"mode": 5` strip should yield 6 indices; a 5-vertex `"mode": 6` fan should yield 9. Two shapes worth covering: - a strip with an **odd** triangle count, to pin the winding-parity rule rather than just the vertex count; - a strip/fan with **no** `NORMAL` attribute, so the asserted normals can only come from the expanded indices reaching the triangle-geometry path. ## Open questions - How common are strip/fan assets in the wild? glTF exporters default to `TRIANGLES`, so this may be a long tail — and some pipelines run assets through meshopt, which normalizes to triangles anyway. Worth measuring against real assets before investing. If the answer is "rare and handled upstream", documenting the limitation may be the better outcome than a fix. - For the non-triangle modes, should the importer warn-and-skip, or should the topology actually be plumbed through? - Does the winding flip's silent truncation of a partial triangle deserve its own issue? It is orthogonal to `mode` and affects indexed `TRIANGLES` too.

SakulFlee added the

Context: Bug

label

2026-09-30 01:25:23 +00:00

SakulFlee self-assigned this

2026-09-30 01:25:23 +00:00