Update version #191

lasalvavida · 2016-12-05T22:16:29Z

@lilleyse and @pjcozzi could you look at this when you get a chance? ~~This refactors any glTF 0.8 ->1.0 from gltfDefaults out into a global stage called updateVersion.~~ (This is now in #223) This stage also includes tools for converting from glTF 1.0 -> 2.0.
~~but lib/Pipeline.js still targets 1.0 by default. This will be changed once glTF 1.1 is fully released.~~

See KhronosGroup/glTF#605 for the full list of changes from glTF 1.0 -> 1.1

@lexaknyazev, if you have time I'd appreciate your input here as well.

TODO

Test new stage with all glTF sample models

…f-pipeline into updateVersion

pjcozzi · 2016-12-06T15:54:05Z

@lasalvavida have you tested this with all the sample models?

lasalvavida · 2016-12-06T15:57:03Z

@lasalvavida have you tested this with all the sample models?

Not yet.

pjcozzi · 2016-12-06T15:58:46Z

@lasalvavida have you tested this with all the sample models?
Not yet.

Added to the tasklist at the top of this PR. Please add any other items there needed before we should merge this. Updating the entire pipeline to 1.1 can be a separate PR if you prefer.

pjcozzi · 2016-12-06T16:02:13Z

@lexaknyazev if you are able to look at this, the two files to review are:

updateVersion.js
updateVersionSpec.js - tests

pjcozzi · 2016-12-06T16:04:55Z

specs/lib/updateVersionSpec.js

+        expect(gltf.asset.profile.version).toEqual('1.0');
+        var animation = gltf.animations.animation;
+        var sampler = animation.samplers.sampler;
+        expect(sampler.input).toBe('accessor_input');


Here and below, should this just be toEqual.

pjcozzi · 2016-12-06T16:17:00Z

specs/lib/updateVersionSpec.js

+        expect(technique.states).toEqual(['TEST_STATE']);
+    });
+
+    it('updates glTF from 1.0 to 1.1', function() {


Can this also test:

glExtensionsUsed and extensionsRequired

buffer.type removal

technique.functions.scissor and technique.states.enable = 3089 removal

Adds all properties that were formerly not required, e.g., asset, buffer.byteLength, bufferView.byteLength, etc.

technique.states.functions.blendColor / technique.states.functions.depthRange parameter changes.

In general, can you look at each bullet in KhronosGroup/glTF#605 and confirm that it is handled? There are others that I did not mention above.

There are some cases where an exact update won't be possible, and we may have to make a breaking change (to the model itself) and provide a log to the user. For example, we may have to prepend an underscore to an application-specific parameter semantic or change camera.perspective.yfov = 0.0 to camera.perspective.yfov = 1.0. In practice, most of these edge cases will not come up.

It is likely cleaner to break this into multiple tests for some of these edge cases.

glExtensionsUsed and extensionsRequired

Those properties have no analogue in older versions of glTF; they are just new properties. I can't take a glTF1.0 extensionsUsed and make it extensionsRequired because we don't know which ones were just used vs. required. As such, I don't think there is any safe updating that can be done here.

buffer.type removal

Thank you, I did miss that one

technique.functions.scissor and technique.states.enable = 3089 removal

Agreed, I will add this.

Adds all properties that were formerly not required, e.g., asset, buffer.byteLength, bufferView.byteLength, etc.

I'll do byteLength, but adding the asset if it doesn't exist is handled by addDefaults.

technique.states.functions.blendColor / technique.states.functions.depthRange parameter changes.

I think this is in the same realm as camera.perspective properties. If someone has a property that is set to an invalid value, I don't think that it is safe to just change the value to a valid one. Even though it was never explicitly forbidden in 1.0, those restrictions were still imposed by WebGL. I can maybe log a warning or something, but I don't think we can change the value.

Actually adding asset if it is not defined is a bit more complex. That being said, I think the existing behavior is the correct approach. If a glTF 0.8 without asset defined is updated, but it has gltf.version, the asset property will be added. However, for any other this will throw an exception, because if asset isn't defined, then asset.version isn't defined, and there is no safe way to determine the version of the glTF, and what changes need to be made.

If it's undefined, we could assume 1.0. I'm just concerned that this could cause some strange behavior down the line somewhere.

I can't take a glTF1.0 extensionsUsed and make it extensionsRequired because we don't know which ones were just used vs. required

I think we do know which were required; it is the extensions that are now required, which I suppose is all of the current extensions.

technique.states.functions.blendColor / technique.states.functions.depthRange parameter changes.

I think this is in the same realm as camera.perspective properties.

It's a tough call, but the 1.0 -> 1.1 stage should output a valid 1.1 model so I would be fine with clamping the value and providing a warning log. There's an OK chance that is what the user wants; otherwise, the user would have to edit the model afterward 100% of the time.

If it's undefined, we could assume 1.0. I'm just concerned that this could cause some strange behavior down the line somewhere.

Probably OK. In practice, I suspect virtually 0% of users use 0.8, and post 1.0, asset is required.

pjcozzi · 2016-12-06T16:20:24Z

lib/updateVersion.js

+    return gltf;
+}
+
+function glTF_0_8_to_glTF_1_0(gltf) {


Even for private functions, we don't put underscore in names.

Do you have a suggested alternative? I was having a hard time making it still readable without underscores.

ha. I had a few then I deleted them. I dunno:

glTF08To10

glTF08ToglTF10

pjcozzi · 2016-12-06T16:23:42Z

lib/updateVersion.js

+    }
+}
+
+function glTF_1_0_to_glTF_1_1(gltf) {


Consider breaking this function up into several smaller ones. Basically each comment could call a separate function.

pjcozzi · 2016-12-06T16:23:52Z

Good start, thanks @lasalvavida!

…f-pipeline into updateVersion

lasalvavida · 2016-12-14T22:54:45Z

This is updated from the discussion above.

pjcozzi · 2016-12-15T16:01:37Z

Great, will aim to review tomorrow morning.

pjcozzi · 2016-12-16T14:05:18Z

Do we handle this one from KhronosGroup/glTF#605:

Attribute parameters can't have a value defined in the technique or parameter, #563 (comment).

I think we should strip the value and warn in this case.

pjcozzi · 2016-12-16T14:11:40Z

Do we handle this from KhronosGroup/glTF#605

Properties in technique.parameters must be defined in technique.uniforms or technique.attributes,
#563 (comment).

Would be possible to know to put a parameter in uniforms or attributes? I suspect this would be painful, and perhaps we could just warn that the parameter is missing from uniforms or attributes. I don't want to overlap too much with the glTF Validator, but case could be OK.

This is also likely to a rare case so no need to spend too much time here.

pjcozzi · 2016-12-16T14:12:32Z

Is this taken into account?

technique.parameter.count can only be defined when the semantic is JOINTMATRIX or an application-specific semantic is used. It can never be defined for attribute parameters; only uniforms, d2f6945

pjcozzi · 2016-12-16T14:14:44Z

Is this taken into account?

technique.parameter.semantic is required when the parameter is an attribute, 28e113d

In practice, it is likely to be rare so no need to over invest here.

We could either (1) warning or (2) make a best attempt to map attribute names to semantics and still warn.

pjcozzi · 2016-12-16T14:18:06Z

I reviewed KhronosGroup/glTF#605 and that is all my comments about how this maps to the 1.1 spec. Please keep an eye on 1.1 issues in the glTF repo for any last-minute updates.

I'll also take a quick look at this code now.

pjcozzi · 2016-12-16T14:31:23Z

lib/Pipeline.js

@@ -84,6 +85,9 @@ Pipeline.processJSONWithExtras  = function(gltfWithExtras, options) {
    if (options.removeNormals) {
        removeNormals(gltfWithExtras);
    }
+    updateVersion(gltfWithExtras, {
+        targetVersion: '1.0'


Should this be 1.1?

Or is this dependent on updating the rest of the pipeline stages to use 1.1? This could be a separate branch, but probably needs to go into master at the same time so we are able to optimize sample models and gltf-pipeline can process models that it outputs.

For now, this is set to 1.0 intentionally since I don't think we want to be generating 1.1 from master by default just yet. Though now that I think about it, a command line flag for 1.1 may be a better solution.

You are right. The other option is to keep the 1.1 changes on this branch, but I agree it would be better to get this into master.

pjcozzi · 2016-12-16T14:41:11Z

lib/updateVersion.js

+    if (defined(profile)) {
+        var version = profile.version;
+        if (defined(version)) {
+            profile.version = version[0] + '.' + version[2];


Shouldn't version[2] be version[1]?

KhronosGroup/glTF#701

And to be safe perhaps the check should be

if (defined(version) && (version.length > 2)) {

This is correct as is, but a length check is a good idea.

0 1 2 3 4

1 . 0 . 3

and we want 1.0, so version[0] + '.' + version[2].

pjcozzi · 2016-12-16T14:45:53Z

lib/updateVersion.js

+                                attributes.TEXCOORD_0 = attributes[semantic];
+                                delete attributes[semantic];
+                            }
+                            if (semantic === 'COLOR') {


minor, but } else if (semantic === 'COLOR') { would be slightly better style.

pjcozzi · 2016-12-16T14:47:49Z

lib/updateVersion.js

+    COLOR: true,
+    JOINT: true,
+    WEIGHT: true,
+    BATCHID: true


BATCHID is part of 3D Tiles, not glTF, so the 3D Tiles spec will need to name this _BATCHID (please update the spec if it isn't already), and then we can have backwards compatibility loading in Cesium with a warning (also please open PR if this isn't done).

pjcozzi · 2016-12-16T14:51:45Z

@lasalvavida just those comments. What is your plan from here?

Perhaps get this fully into shape, but then do not merge until we have the samples converted and Cesium loader so that we know the full stack works together?

…ne into updateVersion

coveralls · 2017-03-21T16:53:32Z

Coverage decreased (-1.5%) to 92.321% when pulling 9fc99cc on updateVersion into bf401ef on master.

coveralls · 2017-03-21T22:18:39Z

Coverage decreased (-1.8%) to 92.049% when pulling 684c272 on updateVersion into bf401ef on master.

coveralls · 2017-03-21T22:31:59Z

Coverage decreased (-1.8%) to 92.03% when pulling d426433 on updateVersion into bf401ef on master.

coveralls · 2017-03-21T22:39:17Z

Coverage decreased (-1.8%) to 92.03% when pulling d426433 on updateVersion into bf401ef on master.

coveralls · 2017-04-02T23:43:50Z

Coverage decreased (-1.9%) to 91.944% when pulling 34d1c67 on updateVersion into bf401ef on master.

coveralls · 2017-04-02T23:48:18Z

Coverage decreased (-1.9%) to 91.952% when pulling 34d1c67 on updateVersion into bf401ef on master.

lasalvavida · 2017-04-28T14:21:14Z

I am closing this here and will re-open as the 2.0 pull request

lasalvavida added 2 commits December 5, 2016 17:09

Added updateVersion stage as well as some refactoring

a5f1ad8

Merge branch 'master' of https://github.com/AnalyticalGraphicsInc/glt…

74c6635

…f-pipeline into updateVersion

lasalvavida mentioned this pull request Dec 5, 2016

Support glTF 2.0 #188

Closed

2 tasks

pjcozzi reviewed Dec 6, 2016

View reviewed changes

pjcozzi mentioned this pull request Dec 6, 2016

New glTF models KhronosGroup/glTF-Sample-Models#9

Merged

lasalvavida added 5 commits December 14, 2016 15:40

Split out into multiple functions

b4ff281

Merge branch 'master' of https://github.com/AnalyticalGraphicsInc/glt…

96160e5

…f-pipeline into updateVersion

Cleaned up a few failing tests

318d2b8

Added new upgrades to specs

77cd995

Application specific parameters are prefixed with underscores

6478c98

pjcozzi reviewed Dec 16, 2016

View reviewed changes

Removed findUsedIds

13aa73d

pjcozzi mentioned this pull request Mar 17, 2017

Update sample models to 2.0, add validator script KhronosGroup/glTF-Sample-Models#37

Closed

29 tasks

lasalvavida and others added 8 commits March 20, 2017 13:15

WIP, switching workspaces

b5d8002

WIP, pretty much just AO left

6427738

Fixed a few more tests

af0a51a

All tests pass

a4a0331

Removed riggedSimpleUnoptimized

94ee8e5

Delete generateTangentsBitangents models

d75085e

Merge branch 'master' of github.com:AnalyticalGraphicsInc/gltf-pipeli…

37b97c0

…ne into updateVersion

Don't look for slots in array, just append

9fc99cc

lasalvavida added 5 commits March 21, 2017 12:58

Small fixes from testing

d172439

More fixes

d4ccb41

A few test fixes and byteStride -> bufferView

edee467

A few more test fixes

2e7519e

Missed a function

684c272

Removed vestigial byteStride references

d426433

lasalvavida added 2 commits March 22, 2017 16:02

Some fixes from cesium changes

a35e5f7

Fixed cesium dependency list

7492f57

pjcozzi mentioned this pull request Mar 28, 2017

Update samples to gltf2.0 specification CesiumGS/3d-tiles-samples#15

Closed

lasalvavida added 2 commits April 2, 2017 19:38

Fixed some plurality issues

ab6daee

Add KHR_technique_webgl extensions if there are techniques

34d1c67

pjcozzi mentioned this pull request Apr 17, 2017

Spec changes from 1.0 to 2.0 KhronosGroup/glTF#605

Closed

lasalvavida closed this Apr 28, 2017

Update version #191

Update version #191

Conversation

lasalvavida commented Dec 5, 2016 • edited

pjcozzi commented Dec 6, 2016

lasalvavida commented Dec 6, 2016

pjcozzi commented Dec 6, 2016

pjcozzi commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasalvavida Dec 6, 2016 • edited

Choose a reason for hiding this comment

lasalvavida Dec 6, 2016 • edited

Choose a reason for hiding this comment

pjcozzi Dec 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Dec 6, 2016

lasalvavida commented Dec 14, 2016

pjcozzi commented Dec 15, 2016

pjcozzi commented Dec 16, 2016

pjcozzi commented Dec 16, 2016

pjcozzi commented Dec 16, 2016

pjcozzi commented Dec 16, 2016

pjcozzi commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasalvavida Dec 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Dec 16, 2016

coveralls commented Mar 21, 2017

coveralls commented Mar 21, 2017

coveralls commented Mar 21, 2017

coveralls commented Mar 21, 2017

coveralls commented Apr 2, 2017

coveralls commented Apr 2, 2017

lasalvavida commented Apr 28, 2017

lasalvavida commented Dec 5, 2016 •

edited

lasalvavida Dec 6, 2016 •

edited

lasalvavida Dec 6, 2016 •

edited

pjcozzi Dec 6, 2016 •

edited

lasalvavida Dec 16, 2016 •

edited