KhronosGroup / glTF-Sample-Assets

To store all models and other assets related to glTF
255 stars 30 forks source link

Animated fireflies and point lights #122

Closed echadwick-artist closed 1 month ago

echadwick-artist commented 1 month ago

screenshot_Large

echadwick-artist commented 1 month ago

Illustration of some of the fixes... smoother shading on stems, red color added to stem tips, firefly pivots improved.

before-after

emackey commented 1 month ago

Awesome work @echadwick-artist!

Given that this is an upgrade for an existing model, and is not introducing any new extension that wasn't already present in the repo, I think this can just be merged. I really love that this is introducing animated point lights, and an animated camera, both of which were sorely lacking from our sample models until now.

I'll note that this new version breaks Filament (but I only tested WASM Filament). It breaks in the Standalone Filament glTF Viewer and in the Filament engine on VSCode, where the whole material lighting model appears broken or incorrect. But it works (to the best of other engines' abilities) on Babylon, Three, Cesium, and Playcanvas, which are the other ones I've tested so far. I don't see any validation errors in the model, so I think it's OK.

The animation and the lights & camera are really great, even for engines that don't offer diffuse transmission. Awesome!

zeux commented 1 month ago

Can the asset be moved to something like glTF-WebP folder? Not all renderers or pipelines support WebP; this would mirror existing precedent with KTX-BasisU assets.

I'm also not sure if webp was intentionally used here; this:

not introducing any new extension

... is not correct because this asset is the first in this repository to introduce usage of EXT_texture_webp without fallbacks, so it can't be rendered without support of this extension.

echadwick-artist commented 1 month ago

But isn’t this also true of any other extensions listed in extensionsRequired? How do we know the penetration of each extension throughout the ecosystem?

For example what if KHR_lights_punctual was in extensionsRequired? It might also not be implemented in all renderers or pipelines.

I don’t like fallbacks because they bloat a file significantly. Should the fallback textures be lower resolution and/or heavily compressed to avoid this problem?

Regardless, I can easily make a JPG/PNG version instead of WEBP, if that’s desired.

echadwick-artist commented 1 month ago

I’m also using webp without fallbacks in PR #123

DRx3D commented 1 month ago

I think it is sufficient to have a good quality image showing the extension in "action" if it is required. That was someone can see what it should look like. Also the 'required' and 'use' extensions are listed in the model's README.

zeux commented 1 month ago

For example what if KHR_lights_punctual was in extensionsRequired? It might also not be implemented in all renderers or pipelines.

I guess from my perspective there should be a difference between using extensions that are necessary to demonstrate the feature that the asset is built for (in this case, lights/materials) and extensions that are compressing data in an alternate way (in this case, webp). It feels better to reserve compression extensions for dedicated model variants so that the base model is maximally "pure".

There's also a difference in behavior in practice between lack of KHR_lights_punctual support (can't render lights) and lack of EXT_texture_webp support given no fallback (can't open any images, a much more prominent issue).

Note: I'm not advocating adding a fallback to PNG/JPG here. I'm advocating one of:

  1. Rename folder to glTF-WebP so that external tools can clearly distinguish this as a variant
  2. Change webp textures to jpg/png (there might be a transmission size impact of course)
  3. Combine 1 plus 2 if desired so that both glTF and glTF-WebP variants are available
emackey commented 1 month ago

@echadwick-artist For what it's worth, KHR_lights_punctual should never be placed in extensionsRequired, because the implicit fallback is simply to ignore the lights. See the final paragraph of Extension Mechanics which is unfortunately non-normative. At some point we might want to go through our list of extensions and add explicit guidance on extensionsRequired for each one.

That said though, I don't know if this sample asset repo has any guidance on whether extensionsRequired (such as WebP with no fallback) is allowed in the basic glTF folder. The tooling here does make a bold assumption that the glTF folder is always present (and broke on a recent submission, I'll file a separate issue on that).