KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.13k stars 1.13k forks source link

Relax requirement for morph target's POSITION? #1357

Closed lexaknyazev closed 5 years ago

lexaknyazev commented 6 years ago

The spec says this for morph target's POSITION:

POSITION accessor must have min and max properties defined.

I don't think this requirement serves any good since these are bounds of deltas of positions. Adding them to the mesh's bounding box won't produce the bounding box of a morph target.

Is there any implementation that actually uses these values?

/cc @bghgary @donmccurdy @emackey

donmccurdy commented 6 years ago

position.min + targetPosition.min and position.max + targetPosition.max still give a valid bounding box, just not a minimal/tight one, yes? It doesn't seem useless to me, but admittedly we're not using them in our implementation.

lexaknyazev commented 6 years ago

just not a minimal/tight one

Depending on values, it may be completely useless: think of a polygon-to-star morphing.

emackey commented 6 years ago

Not sure I understand. Could morphing ever produce a final position that was higher than @donmccurdy's position.max + targetPosition.max ? If not, I would think this would still be useful for non-tight bounding volume computations.

That said I don't know much about the morphing implementation in Cesium or whether these are used there.

lexaknyazev commented 6 years ago

Could morphing ever produce a final position that was higher...

I can't think of any example of that. Nevertheless, I'm questioning usefulness of a "non-tight" volume computed this way.

Original figure image

Real bounding box from positions: (-0.95; -0.81) -> (0.95; 1.00), area: 3.44

Deltas (clockwise, starting from the top) image

Deltas bounds: (-0.59; -0.62) -> (0.59; 0.50)

Morphed figure image

Real bounding box from positions: (-0.95; -1.00) -> (0.95; 1.00), area: 3.80

Computed bounding box from deltas bounds: (-1.54; -1.43) -> (1.54; 1.50), area: 9.00

donmccurdy commented 6 years ago

I think I'd still be comfortable using these (oversized) bounds for things like frustum culling or switching LODs; but certainly that's no good for a physics bounding box...

emackey commented 6 years ago

Yes, loose bounding volumes are useful in Cesium where the render space is potentially the entire inner Solar System. The bounding volume is just used to cull things that are easy to optimize away. Having the volume be larger by 6 meters isn't a big deal at all. Being larger by 300% isn't ideal, but is still better than "infinity" as a bounding volume. However, it's not allowed to be smaller than the transformed object, otherwise the object could get culled inappropriately.

donmccurdy commented 5 years ago

I think I’d vote to keep this requirement.