Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Added generated 2.0 sample models #28

Merged
merged 1 commit into from
May 11, 2017
Merged

Added generated 2.0 sample models #28

merged 1 commit into from
May 11, 2017

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Jan 26, 2017

glTF 2.0 models (spec still in progress): https://github.com/lasalvavida/glTF-Sample-Models/tree/2.0/2.0


Generated from CesiumGS/gltf-pipeline#191.

There may still be some changes to these, but they are at least valid 1.1 as per the validator.

These are optimized and the image assets have been minified. The 2.0 folder is ~76MB vs the 1.0 folder which is ~258MB.

@pjcozzi
Copy link
Member

pjcozzi commented Jan 27, 2017

This all sounds great, thanks @lasalvavida!

Let's keep this PR open and squash git history once the spec and files stabilize.

@lasalvavida
Copy link
Contributor Author

@pjcozzi This is updated with the 2.0 sample models generated from KhronosGroup/COLLADA2GLTF#20

@pjcozzi
Copy link
Member

pjcozzi commented Feb 13, 2017

Very nice, thanks @lasalvavida.

Can you please add a README.md to the 2.0 directory and each model's directory?

Why were the 1.0 files deleted? Were these extra files? We still want to keep 1.0 assets in this repo.

@pjcozzi pjcozzi mentioned this pull request Feb 13, 2017
23 tasks
@lasalvavida
Copy link
Contributor Author

lasalvavida commented Feb 13, 2017

I may need to squash for it to appear correctly. Collada source files were moved out of 1.0 into their own separate folder.

@cx20
Copy link
Contributor

cx20 commented Feb 15, 2017

@lasalvavida GearboxAssy does not exist. Is it a mistake?

@lasalvavida
Copy link
Contributor Author

@lasalvavida GearboxAssy does not exist. Is it a mistake?

Yes it is a mistake. I will make sure it's there on the next round of regenerations.

@donmccurdy
Copy link
Contributor

Hi @lasalvavida, I'm testing these against THREE.GLTFLoader and THREE.GLTF2Loader, and there's a change in the structure I wasn't expecting. Referencing RiggedSimple.gltf#L11-L92, the hierarchy of the glTF2.0 model is:

- defaultScene
  - Y_UP_Transform
    - Proxy
- [NO PARENT SCENE]
    - Armature
      - torso_joint_1
      - ...

The glTF1.0 asset by contrast includes both the Armature and the Proxy as descendants of the default scene:

- defaultScene
  - Y_UP_Transform
    - Armature
      - torso_joint_1
      - ...
    - Proxy

Is this an intended change? I could perhaps see an argument that the armature and joints would not be part of the scene necessarily, but it does break our current skinning implementation for three.js.

@lasalvavida
Copy link
Contributor Author

Hi @donmccurdy!

There are actually two changes here.

The first is that skeleton nodes must be the root of a node hierarchy. That is why Armature is no longer a child of Y_UP_Transform.

I don't think it is technically incorrect to list a skeleton node in scene nodes, but it is not explicitly required, and wasn't in 1.0 either. See: https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/SimpleSkin

In this case, it is redundant to list the skeleton node in scene nodes because it is referenced as a skeleton. We had to make some changes in Cesium to accommodate this as well.

@lexaknyazev
Copy link
Member

@lasalvavida

The first is that skeleton nodes must be the root of a node hierarchy.

It's no longer the case with 2.0.

@lasalvavida
Copy link
Contributor Author

Ah, thank you for the clarification @lexaknyazev. My other point is correct though, right? Listing skeleton nodes in scene is unnecessary.

@lasalvavida
Copy link
Contributor Author

Also, the 2.0 glTF branch still says: The skeletons property contains one or more skeletons, each of which is the root of a node hierarchy.

@lexaknyazev
Copy link
Member

@lasalvavida
Here's relevant section:

The skeleton property points to node that is the root of a joints hierarchy.

"Root of joints" could be non-root scene-wise.

@lasalvavida
Copy link
Contributor Author

My mistake, I was looking at 1.0 in the 2.0 branch.

@donmccurdy
Copy link
Contributor

Thanks for the clarification @lasalvavida and @lexaknyazev. Sounds good to me that the skeleton node does not need to be in a scene.

I've run into one other difference: If The skeleton property points to node that is the root of a joints hierarchy, is that root node itself a joint? As of 1.0 it was, and with 2.0 it is not.

@emackey
Copy link
Member

emackey commented Apr 4, 2017

Seems like this has a huge number of changes that have been open a long time, and other folks are filing pull requests directly against master. Should this be merged soon? Or abandoned and re-converted at some point?

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Apr 4, 2017

@emackey, I should have this updated today/tomorrow once I finish my testing. A 2.0 branch is probably the preferred solution since the spec is not officially ratified yet.

edit: Actually since we have the 2.0 folder in master already, they should probably just go in there

double edit: I had some stuff come up that I've been needing to deal with, but this will definitely be done on Friday.

@emackey
Copy link
Member

emackey commented Apr 4, 2017

@lasalvavida Sounds good, just be aware that master already has a 2.0 folder, and other pull requests such as #40 are targeting that folder in master.

@lasalvavida
Copy link
Contributor Author

These are updated from the latest COLLADA2GLTF-2.0 which includes PBR and pbrSpecularGlossiness

@pjcozzi
Copy link
Member

pjcozzi commented Apr 11, 2017

@lasalvavida looks like this still has a 1.1 directory?

@lasalvavida
Copy link
Contributor Author

I need to merge in master, after that it should be fine

@lasalvavida
Copy link
Contributor Author

This PR has been regenerated from the latest COLLADA2GLTF-2.0 build

@emackey
Copy link
Member

emackey commented Apr 28, 2017

@pjcozzi can this be merged soon? There are a number of other draft 2.0 models already in a 2.0-named folder in master, from multiple contributors. I don't think the 2.0 folder would get removed from master before ratification, so it would be great to consolidate there by merging this in. When the 2.0 spec is finally ratified, there will need to be another pass to make sure all sample models already in master are in line with what was ratified, regardless if this was merged by then or not. Let's not wait.

@cx20
Copy link
Contributor

cx20 commented Apr 28, 2017

@lasalvavida GearboxAssy has not been generated yet.

@lasalvavida
Copy link
Contributor Author

@cx20, that's not correct, GearboxAssy is present in this pull request

https://github.com/lasalvavida/glTF-Sample-Models/tree/2.0/2.0/GearboxAssy

@cx20
Copy link
Contributor

cx20 commented Apr 28, 2017

@lasalvavida I'm sorry. It is my mistake. It is OK now.

@cx20
Copy link
Contributor

cx20 commented Apr 28, 2017

@lasalvavida In the latest version generator, .jpg seems to be used instead of .png. I think it is okay to delete .png.

https://github.com/lasalvavida/glTF-Sample-Models/tree/2.0/2.0/BarramundiFish/glTF-pbrSpecularGlossiness

BarramundiFish_metallicRoughness.png
BarramundiFish_texture_0001.png
BarramundiFish_texture_0002.png
BarramundiFish_texture_0003.png
BarramundiFish_texture_0004.png

@emackey
Copy link
Member

emackey commented Apr 28, 2017

@lasalvavida it looks like these models still have metallic in the R channel, not B. The draft spec was updated to B, has this been taken into account in the converter?

@lasalvavida
Copy link
Contributor Author

@emackey, the metallicRoughness textures were pulled from @sbtron's PBR models; I'll look around and see if he has an updated set, otherwise I'll just do it myself.

COLLADA has no concept of PBR, so metallicRoughness texture paths are passed in as a command line argument, not generated.

@emackey
Copy link
Member

emackey commented Apr 28, 2017

https://github.com/sbtron/BabylonJS-glTFLoader/tree/master/src/models/2.0

@lasalvavida
Copy link
Contributor Author

Good eye @cx20!

README.md Outdated
@@ -5,7 +5,7 @@
# glTF Sample Models

- [glTF 1.0](1.0)
- [glTF 2.0](2.0)
- [glTF 1.1](1.1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a regression here, maybe a bad merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, looks like a bad merge

"sampler": 0,
"source": 3,
"target": 3553,
"type": 5121
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format, internalFormat, target, and type have all been removed from 2.0.

@cx20
Copy link
Contributor

cx20 commented Apr 29, 2017

I am testing generated 2.0 sample models.
https://github.com/cx20/gltf-test/tree/2.0
However, Duck.gltf seems to be very big compared to the 1.0 model.
If you set the scale to 1/100 on the library side, it will be correct displayed, but it is unknown whether it is a library problem or a model problem.

glTF 1.0 Duck.gltf Scale 1/1:
https://cx20.github.io/gltf-test/examples/threejs/index.html?model=Duck&scale=1
glTF 2.0 Duck.gltf Scale 1/1:
https://cdn.rawgit.com/cx20/gltf-test/438b789d08909b233ee8fb16c1801407f10acd14/examples/threejs/index.html?model=Duck&scale=1
glTF 2.0 Duck.gltf Scale 1/100:
https://cdn.rawgit.com/cx20/gltf-test/438b789d08909b233ee8fb16c1801407f10acd14/examples/threejs/index.html?model=Duck&scale=0.01

@javagl
Copy link
Contributor

javagl commented Apr 29, 2017

@cx20 and @lasalvavida : This difference is likely caused by the camera. I did not verify this in depth, but it was my first guess, and it's very likely the reason: The camera for the duck in 1.0 is

    "cameraShape1": {
        "name": "cameraShape1",
        "perspective": {
            "aspectRatio": 1.5,
            "yfov": 0.660593,
            "zfar": 100,
            "znear": 0.01
        },
        "type": "perspective"
    }

The camera for the duck in 2.0 contains some really strange values

    {
        "perspective": {
            "aspectRatio": 2169.69921875,
            "yfov": 2169.69921875,
            "zfar": 10000.0,
            "znear": 1.0
        }
    }

The FOV and aspect ratio are certainly not right...

@cx20
Copy link
Contributor

cx20 commented Apr 29, 2017

@javagl @lasalvavida I tried changing the value of the camera of gltf 2.0 to the value of 1.0, but the result did not change. I think not a camera problem. When I check the buffer of the Duck with WebGL Inspector and it seems that the scale of vertex data is 100 times. Is that the cause?

glTF 1.0 Duck.gltf and Three.js glTF 1.0 Loader:
image

glTF 2.0 Duck.gltf and Three.js glTF 2.0 Loader:
image

@lasalvavida
Copy link
Contributor Author

Those camera values are certainly wrong. @cx20, I think that comes from this part of the COLLADA:

<unit meter="0.01" name="centimeter"/>

The old converter used to bake these asset scales into the data; I will be adding a scale to the root transform to reflect this which should correct the issue, but the data in the buffer will still be different.

@pjcozzi
Copy link
Member

pjcozzi commented May 3, 2017

@pjcozzi can this be merged soon? There are a number of other draft 2.0 models already in a 2.0-named folder in master, from multiple contributors. I don't think the 2.0 folder would get removed from master before ratification, so it would be great to consolidate there by merging this in...

I'm OK with merging this whenever folks are happy, and making another pass for final glTF 2.0 cleanup.

Please bump when it should be merged.

@lasalvavida
Copy link
Contributor Author

Updated WEIGHT and JOINT -> WEIGHTS_0 and JOINTS_0. Asset scales are accounted for and the garbage values in the cameras are corrected. Updated metallicRoughness textures. Redundant images have been removed

@pjcozzi
Copy link
Member

pjcozzi commented May 10, 2017

@lasalvavida is this in shape to merge?

@emackey
Copy link
Member

emackey commented May 10, 2017

README.md still has a very minor bad merge. I haven't taken a detailed look at this latest incarnation but at a cursory glance things seem OK. We can make new issues after the merge of course.

@lasalvavida
Copy link
Contributor Author

Okay, I'll update the readme and squash and then unless anybody has anything else this should be okay to merge

@lasalvavida
Copy link
Contributor Author

Updated

@emackey
Copy link
Member

emackey commented May 11, 2017

The README in the 2.0 folder still says

**Note:** These models are not yet updated to glTF 2.0!

Also several models are missing from that readme.

@emackey
Copy link
Member

emackey commented May 11, 2017

I'm looking through the models, they look good so far.

@lasalvavida
Copy link
Contributor Author

Note: These models are not yet updated to glTF 2.0!

That's from the current master. I'm pretty sure it refers to @javagl's simple models not being updated yet.

Which models are missing? I'm looking at: https://github.com/lasalvavida/glTF-Sample-Models/blob/2.0/2.0/README.md and I'm not sure what you mean

@emackey
Copy link
Member

emackey commented May 11, 2017

Ah. The missing ones aren't from this PR, they're mostly mine (like MetalRoughSpheres). I can open a separate PR to fix that.

I haven't tested each and every model, but I'm starting to think this PR is good to go.

@pjcozzi
Copy link
Member

pjcozzi commented May 11, 2017

Very nice! 🎆

@pjcozzi pjcozzi merged commit 0589a6c into KhronosGroup:master May 11, 2017
@javagl
Copy link
Contributor

javagl commented May 12, 2017

Regarding the models that are not 2.0 yet: Most of them will be, as of PR #45 (Three of them are still not updated, and they'll be marked as such directly in the table of the README. These are the material-related ones, and I'm still working on that. If it takes too long, I'll update the PR to omit these models. There are more, better, "real" PRB-example models now...)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants