-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VC model is broken; displays flying boxes. #71
Comments
Mentioned in KhronosGroup/glTF#1982 (comment), this model also uses the same textures for base color and occlusion, which must be a mistake in the conversion as well... |
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".
|
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? |
|
I also like this model for the same reason as @javagl. |
Here's another attempt at fixing the technical issues (not so much the artistic issues) programmatically: 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()
); 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. 😁 |
@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...) |
Just guessing:
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 |
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) |
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 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. |
Long story short, I would keep the COLLADA file, and put a copy of Don's code snippet in a text file alongside it. |
BTW, I found the original display results in the asset store. Indeed, it seems that the windshield of the original model was transparent. |
One more update to make the windshield orange again: convert.tsimport { 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()
); |
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. |
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 KhronosGroup/glTF-Sample-Models#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... |
I completely agree with your opinion. |
+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). |
- Add camera switching button for models with more than 1 camera - Can be tested with fixed VC-v2.glb files posted by donmccurdy here: https://github.com/KhronosGroup/glTF-Sample-Models/issues/275
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. |
This issue was originally about a COLLADA conversion error, and we don't have COLLADA ( 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. |
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:
Plan A:
Plan B:
|
The PR is at #89 |
See: KhronosGroup/COLLADA2GLTF#146
https://cx20.github.io/gltf-test/examples/threejs/index.html?model=VC&scale=0.2
The text was updated successfully, but these errors were encountered: