KhronosGroup / glTF-Sample-Assets

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

VC model is broken; displays flying boxes. #71

Open donmccurdy opened 3 years ago

donmccurdy commented 3 years ago

See: https://github.com/KhronosGroup/COLLADA2GLTF/issues/146

https://cx20.github.io/gltf-test/examples/threejs/index.html?model=VC&scale=0.2

donmccurdy commented 3 years ago

Mentioned in https://github.com/KhronosGroup/glTF/issues/1982#issuecomment-840183232, this model also uses the same textures for base color and occlusion, which must be a mistake in the conversion as well...

javagl commented 3 years ago

This model causes a lot of issues, and analyzing and fixing them is a pain in the back. On top of that, it's not always clear whether a certain issue is caused by the source model, COLLADA2GLTF, or the renderer that is used (although the latter could be checked relatively easily, thanks to cx20's test suite).

But it's still one of my favorite models (if not the favorite model), because it combines nearly all features of glTF (1.0, FWIW). I think it's the only model that has multiple cameras, certainly the only one with animated cameras, fancy material settings (for the windshields), and ... heck, it just looks cool.

I'd be willing to allocate some time to bring this into a shape that is "less buggy". But ... how successful that could be depends on many unknowns

emackey commented 3 years ago

This model shows a dark city with a ton of illuminated windows, yet makes no use of the emissive channel or metallic surfaces. It's a lackluster example of how broken materials were for 3D model files in general before glTF 2.0's core PBR hit the market.

I don't particularly like this model on aesthetic grounds, it looks built with 20-year-old technology. Are there any technical reasons to keep it around?

emackey commented 3 years ago

image

cx20 commented 3 years ago

I also like this model for the same reason as @javagl. The materials are a bit old-fashioned, but the multiple camera transitions combined with the animations are spectacular.

donmccurdy commented 3 years ago

Here's another attempt at fixing the technical issues (not so much the artistic issues) programmatically:

VC-v2.glb.zip

import { dedup, prune, resample } from "@gltf-transform/lib";

// Previously loaded 'document' with NodeIO or WebIO classes.
const root = document.getRoot();

for (const material of root.listMaterials()) {
    // Occlusion textures on this model are a lie. 
    material.setOcclusionTexture(null);

    // Fix transparency on jet windshield.
    if (material.getName() === '_y4y4y4') {
        material
            .setAlpha(0.5)
            .setAlphaMode('BLEND')
            .setRoughnessFactor(0.0);
    }
}

for (const mesh of root.listMeshes()) {
    const name = mesh.getName();

    // Remove flying boxes.
    if (/^cam|^dum|^box|-box/i.test(name)) {
        mesh.dispose();
    }
}

// Clean up.
await document.transform(
    resample(),
    dedup(),
    prune()
);

Screen Shot 2021-05-13 at 11 21 14 AM

That's about as far as I'd want to go in trying to fix it, myself. I tend to agree with Ed that the model is showing its age at this point. We don't have many "whole scenes" in the sample repo at the moment — just VC and Sponza, which has its own issues — so I guess I'm inclined to keep it for now. But I wouldn't be at all sorry to replace it with something a bit more modern someday. 😁

javagl commented 3 years ago

@donmccurdy Do you know from the tip of your head which of these issues are caused by "non-sensical input" or by COLLADA2GLTF? (The point is: Maybe one could do what you did there, like removing the "box" nodes and tweaking some materials, so that the updated DAE can be stored in the repo, and converted to proper glTF with COLLADA2GLTF...)

donmccurdy commented 3 years ago

Just guessing:

  1. The boxes have names like "cam-box" and "dummy" suggesting that — in some prior life of this model — they were intentionally used for something.
  2. The occlusionTexture entries are likely a conversion bug? Or at least, I can't imagine the same texture was really supposed to act as both AO and base color, and arguably this renders the glTF file invalid since the colorspace can't be both linear and sRGB.
  3. The missing transparency on the windshield is anyone's guess..

Fixing the source DAE model sounds like much more work than I'm interested in myself. Doing so would benefit the COLLADA2GLTF project, but as for the sanity of this sample repository, source files in .gltf, .blend (or even .usd) formats would be more future-proof.

javagl commented 3 years ago

Times are changing :-) there once was a reason to keep all the source models in this repo as well.

I'm not sure whether there are any claims (or goals) like ~"The glTFs should be (exactly) the result of applying COLLADA2GLTF to the (DAE) source models". If this is fixed only in the glTF, then the COLLADA source model may be removed (otherwise, it may cause confusion). On the other hand: If there is an issue in COLLADA2GLTF that shows up for this model in particular, the issue should be fixed and the original model could serve as a test case.

I'll try to allocate some time and see what Blender says about this model, with the disclaimers (regarding timeline and success) mentioned above.

(EDIT: If that takes too long, or does not lead anywhere, there's still the option to 1. just update the glTF with the gltf-transform'ed version and deleting the source, and 2. opening an issue in COLLADA2GLTF to investigate that further)

emackey commented 3 years ago

I've always found it odd: We talk about COLLADA being an interchange format, and glTF being a transmission/delivery format, and the two formats have different design goals with different information encoded in each. Why then are we obsessed with having an automated utility that converts one to the other, untouched by human hands? The Venn Diagram of COLLADA contents and glTF contents has some overlap for basic geometry, but there are things on the COLLADA side that are guaranteed to be lost, and things on the glTF side (including all of PBR) that will never be created by such a process.

Many sample models in this repo display and test draft extensions, or recently ratified extensions, for which automated exporters are not available. The sourceModels folder is more of a collection of raw assets that were used in the creation of a glTF, but I think there cannot be a hard requirement for an automated conversion from raw source assets to finished glTF. For example, it's quite helpful to me to keep the Blender projects and GIMP save files around for things like TransmissionRoughnessTest, even though Blender and GIMP cannot reproduce this model by fully automated means.

In the early development of COLLADA2GLTF, we were re-running automated conversion on a routine basis, as both the converter and the 2.0 spec itself were undergoing massive changes at the time (before 2.0 was ratified). Those days are gone, the 2.0 core spec is established and adopted, and the converter is no longer under active development. We shouldn't still be expecting some automated process to get from a raw source interchange format to a final deliverable art asset. And it can still be helpful to keep the raw interchange format around as reference, just drop the expectation of automated conversion.

emackey commented 3 years ago

Long story short, I would keep the COLLADA file, and put a copy of Don's code snippet in a text file alongside it.

cx20 commented 3 years ago

The missing transparency on the windshield is anyone's guess..

BTW, I found the original display results in the asset store. Indeed, it seems that the windshield of the original model was transparent.

https://3drt.com/store/environments/virtual-city-pack.html

image

donmccurdy commented 3 years ago

One more update to make the windshield orange again:

convert.ts ```ts import { dedup, prune, resample } from "@gltf-transform/lib"; // Previously loaded 'document' with NodeIO or WebIO classes. const root = document.getRoot(); for (const material of root.listMaterials()) { // Occlusion textures on this model are a lie. material.setOcclusionTexture(null); // Fix transparency on jet windshield. if (material.getName() === '_y4y4y4') { material .setAlpha(0.5) .setAlphaMode('BLEND') .setBaseColorHex(0x7A6005) .setRoughnessFactor(0.0) .setDoubleSided(true); } } for (const mesh of root.listMeshes()) { const name = mesh.getName(); // Remove flying boxes. if (/^cam|^dum|^box|-box/i.test(name)) { mesh.dispose(); } } // Clean up. await document.transform( resample(), dedup(), prune() ); ```

VC-v2.glb.zip

bghgary commented 3 years ago

there cannot be a hard requirement for an automated conversion from raw source assets to finished glTF

I think we should try to have source assets and instructions for how to go from source to the final asset, just in case some tweaks need to be made. This shouldn't be a requirement though.

javagl commented 3 years ago

I didn't intend to spark a broader discussion about COLLADA, glTF and the role that COLLADA2GLTF plays between them. And I am certainly not "obsessed" with a certain utility or toolchain for the models in general. However, there are COLLADA assets out there. People want to convert them to glTF, even though there may be features that are lost in translation, or features of glTF that remain unused. The VC model is such an asset. COLLADA2GLTF should be able to convert into a glTF that is technically valid, and ... "visually as nice as possible".

Regarding the role of the model itself: I am (and always have been) advocating to have dedicated "Feature Test"-sample models. Yes, the BoomBox looks nice. The BoxAnimated is useful for tests. But there should also be some models that ~"bring everything together" (and... now this is somehow related to https://github.com/KhronosGroup/glTF-Sample-Models/issues/298 ...). There should be a model that shows a real scene, in addition to the ones that show single, static objects wtith various PBR effects. If some artist created a "VC 2.0", with a complex scene consisting of multiple buildings, animated cameras and animated spacecrafts, that would be great. But except for VC, there is no such model...

cx20 commented 3 years ago

If some artist created a "VC 2.0", with a complex scene consisting of multiple buildings, animated cameras and animated spacecrafts, that would be great.

I completely agree with your opinion.

neiltrevett commented 3 years ago

+1.

I sometimes get told that 'glTF is just good spinning an object'. It would be good to have some compelling assets/tests in the working group samples repo that clearly demonstrate otherwise (while also recognizing that the latest geospatial demos from the likes of Cesium and ESRI are another way of showing glTF use cases include more than spinning a single object).

DRx3D commented 7 months ago

There have been no comments in over 2 years. The basic issue still stands - VC (now called VirtualCity) needs replacing. Should this issue be transferred to Sample Assets or a new one created.

Default action is to transfer the issue by 27 November.

emackey commented 7 months ago

This issue was originally about a COLLADA conversion error, and we don't have COLLADA (sourceModels) in the new repo. So, I think make a new issue on that repo that VC needs upgrading or replacing, and just link to the relevant points here.

I particularly like the comments above that the glTF samples lack something good that's "not just a spinning object," we should be certain to capture that in the new issue.

cx20 commented 6 months ago

@DrX3D

The link is probably broken because the folder name was changed. I think you need to match either the folder name or the file name.

As Is: folde rname file name
VirtualCity/glTF-Binary VC.glb
VirtualCity/glTF-Draco VC.gltf
VirtualCity/glTF-Embedded VC.gltf
VirtualCity/glTF VC.gltf
Plan A: folde rname file name
VirtualCity/glTF-Binary VirtualCity.glb
VirtualCity/glTF-Draco VirtualCity.gltf
VirtualCity/glTF-Embedded VirtualCity.gltf
VirtualCity/glTF VirtualCity.gltf
Plan B: folde rname file name
VC/glTF-Binary VC.glb
VC/glTF-Draco VC.gltf
VC/glTF-Embedded VC.gltf
VC/glTF VC.gltf
javagl commented 6 months ago

Despite all the open questions related to this model, the solution should be that of 'Plan A'.

(Admittedly, I had not looked into https://github.com/KhronosGroup/glTF-Sample-Assets/commit/e5159d0dd6876f220ea180bc7af4b9977a3acfaf by @DrX3D, but maybe just assumed that the file names had been changed as well).

I'll add a TODO on my list and will do that in the next days 🤞

javagl commented 6 months ago

The PR is at https://github.com/KhronosGroup/glTF-Sample-Assets/pull/89