pmndrs / gltfjsx

🎮 Turns GLTFs into JSX components
https://gltf.pmnd.rs
MIT License
4.71k stars 310 forks source link

[BUG] Certain non-empty groups are discarded #102

Closed looeee closed 3 years ago

looeee commented 3 years ago

In the following model, certain non-empty groups are discarded. Pill1_V2.zip

If you open it up in GLTFViewer and check the console, you'll see this structure:

1

However, if I run npx gltfjsx Pill_V2.glb I get this structure:

     <group rotation={[-Math.PI / 2, 0, 0]} scale={100}>
        <group position={[0, 0, 0]}>
          <mesh geometry={nodes.front.geometry} material={nodes.front.material} position={[0, 0, 0]} />
          <mesh geometry={nodes.border.geometry} material={nodes.border.material} position={[0, 0, 0]} />
        </group>
        <mesh geometry={nodes.border_1.geometry} material={nodes.border_1.material} position={[0, 0, 0]} />
        <mesh geometry={nodes.front_1.geometry} material={nodes.front_1.material} position={[0, 0, 0]} />
        <mesh geometry={nodes.circle.geometry} material={materials.mat_pill_circle} position={[0, 0, 0]} />
        <mesh geometry={nodes.front_2.geometry} material={nodes.front_2.material} position={[0, 0, 0]} />
        <mesh geometry={nodes.border_2.geometry} material={nodes.border_2.material} position={[0, 0, 0]} />
      </group>

The center and start groups have been discarded, although end is preserved.

Examining the groups, the only difference I can see is that end has position equal to 1.49011608607807e-10, 0, 0], while center and start have position of [0,0,0].

If I run npx gltfjsx Pill_V2.glb -verbose I get the correct structure:

<group name="Pill1" rotation={[-Math.PI / 2, 0, 0]} scale={100}>
            <group name="end" position={[0, 0, 0]}>
              <mesh name="front" geometry={nodes.front.geometry} material={nodes.front.material} position={[0, 0, 0]} />
              <mesh
                name="border"
                geometry={nodes.border.geometry}
                material={nodes.border.material}
                position={[0, 0, 0]}
              />
            </group>
            <group name="center">
              <mesh
                name="border_1"
                geometry={nodes.border_1.geometry}
                material={nodes.border_1.material}
                position={[0, 0, 0]}
              />
              <mesh
                name="front_1"
                geometry={nodes.front_1.geometry}
                material={nodes.front_1.material}
                position={[0, 0, 0]}
              />
            </group>
            <group name="start">
              <mesh
                name="circle"
                geometry={nodes.circle.geometry}
                material={materials.mat_pill_circle}
                position={[0, 0, 0]}
              />
              <mesh
                name="front_2"
                geometry={nodes.front_2.geometry}
                material={nodes.front_2.material}
                position={[0, 0, 0]}
              />
              <mesh
                name="border_2"
                geometry={nodes.border_2.geometry}
                material={nodes.border_2.material}
                position={[0, 0, 0]}
              />
            </group>
          </group>

However, according to the docs verbose is only supposed to affect empty groups:

--verbose, -v Verbose output w/ names and empty groups

(unrelated: shouldn't the `position={[0,0,0]} be omitted as well?)

drcmda commented 3 years ago

it's going to remove groups that it sees as pointless, either if they're truly empty, or if they don't have any relevant props, for instance position, rotation, etc, so essentially they don't change the outcome. --verbose is the correct setting in your case because it retains the full structure. this is worded wrong, i'll adjust the readme.

    // Remove empty groups
    if (
      !options.verbose &&
      (type === 'group' || type === 'scene') &&
      (result === oldResult || obj.children.length === 0)
    ) {
      return children
    }

the position={[0,0,0]} thing is very weird because it has this check:

    if (obj.position && obj.position.isVector3 && obj.position.length())
      result += `position={[${rNbr(obj.position.x)}, ${rNbr(obj.position.y)}, ${rNbr(obj.position.z)},]} `

am i wrong in assuming that coercing position.length() into a bool will work?

edit:

i believe in this case your vector is not 0, but something like 0.00000000126276...., and the configured precision (2) clamps it to a zero. i think it would make sense to make the check against zero after the clamping, not before, i'll fix this ...

drcmda commented 3 years ago

should be fixed (the issue with pos=0), as for the groups i think that's expected behaviour and i've adjusted the readme.

looeee commented 3 years ago

Hmm, I don't agree that it's correct to assume a group is irrelevant just because it doesn't apply a transform. At least it's not behavior that I'd expect.

In this case the model is a label that will have text placed on it, the groups are there so that I can adjust the width of the labels depending on the text length.

If you do want to keep this behavior I won't argue further, but would it be ok to spell it out more clearly in the readme? Instead of saying "irrelevant" I think it would be clearer to explicitly state "groups that don't have a transform will be dropped unless you use the -verbose flag".

Side note: until now I had ignored the verbose flag because I assumed it was for printing extra data to the console. I think it's slightly unusual nomenclature to have a verbose flag that affects the result rather than just the logs.

looeee commented 3 years ago

i believe in this case your vector is not 0, but something like 0.00000000126276...., and the configured precision (2) clamps it to a zero. i think it would make sense to make the check against zero after the clamping, not before, i'll fix this ...

Yeah that's probably it. The values coming through are very tiny, like 1.49011608607807e-10 from the end group. Essentially they are just rounding errors.

looeee commented 3 years ago

As for the verbose flag, what do you think of splitting it in two? Often I'd want to keep names but not groups and vice-versa. Something like:

--keepnames, -kn --keepgroups, -kg

drcmda commented 3 years ago

I'll implement that

drcmda commented 3 years ago

it's in sorry for the wait