Skip to content
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

2.0.0-dev.1.7 #59

Merged
merged 27 commits into from
Mar 7, 2018
Merged

2.0.0-dev.1.7 #59

merged 27 commits into from
Mar 7, 2018

Conversation

lexaknyazev
Copy link
Member

@lexaknyazev lexaknyazev commented Dec 15, 2017

lexaknyazev and others added 3 commits December 15, 2017 14:22
Spec says: ***When the node contains `skin`, all `mesh.primitives` must contain `JOINTS_0` and `WEIGHTS_0` attributes.***

That makes sense to me, but maybe it's this code that's right, in which case we should update the spec.
@lexaknyazev lexaknyazev added this to the 2.0 milestone Dec 15, 2017
@emackey
Copy link
Member

emackey commented Dec 15, 2017

Is this ready? I see #58 is un-checked above, but it looks like related code is on this branch already.

ISSUES.md needs a refresh, other than that this all looks good.

@lexaknyazev
Copy link
Member Author

@emackey #58 hasn't yet been implemented. I'm going to finish it over the weekend.

@lexaknyazev lexaknyazev mentioned this pull request Feb 24, 2018
@lexaknyazev
Copy link
Member Author

Remaining skinning stuff (joint indices, normalized weights, and hierarchy limits) will go to the next release. This one will be published this week.

@lexaknyazev
Copy link
Member Author

@emackey @donmccurdy
This is ready for review. Please check if language of CHANGELOG.md and ISSUES.md is clear enough.

if (mesh.primitives
.any((primitive) => primitive.jointsCount != 0)) {
context.addIssue(LinkError.nodeSkinnedMeshWithoutSkin);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it valid to specify skin on a node with no mesh at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

new SemanticError._(
'MESH_PRIMITIVES_UNEQUAL_JOINTS_COUNT',
(args) => "All primitives must contain the same number of 'JOINTS' "
"and 'WEIGHTS' attribute sets.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like the idea of error or warning here, but is this mentioned in the spec?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@donmccurdy donmccurdy Mar 6, 2018

Choose a reason for hiding this comment

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

The spec requires each primitive have JOINTS_0 and WEIGHTS_0, but I see nothing to say that one primitive couldn't have JOINTS_1 in addition to JOINTS_0 when other primitives do not. The spec would lead me to think that is valid, while this error would lead me to think it is not.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a data-driven opinion on this. We haven't yet seen models with more than 4 influences per vertex.

What do you think?

Copy link
Contributor

@donmccurdy donmccurdy Mar 6, 2018

Choose a reason for hiding this comment

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

About whether it should be allowed, or whether the validator should say something about it?

I haven't seen any examples either. I'd prefer that primitives be more consistent than not, but elsewhere we've asserted primitives could have different attribute sets without discussing skinning specifically...

The message is sort of saying one thing and testing another, but tentatively, I'd lean toward making the error code something more generic (MESH_PRIMITIVES_INVALID_JOINTS_COUNT) and then we can change either the behavior of the test or the message text depending on what we decide in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd lean toward making the error code something more generic

Agree.

@zellski do you have preference on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking -- no, no strong preference. I feel mildly irked by the thought of completely heterogenous primitives in a mesh, but I have no rational basis for that irritation. When we implement animation at Facebook, I expect we'll treat primitives in a mesh as almost completely unrelated to one another, so no strong "day job" preference either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Considering that skinned mesh transform should be ignored, all primitives of the skinned mesh must have joints. Whether they have to have the same number of influences per vertex is a different question.

I'll update the code and tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out we have a separate issue for skinned primitives without joints:

if (mesh.primitives
.any((primitive) => primitive.jointsCount == 0)) {
context.addIssue(LinkError.nodeSkinWithNonSkinnedMesh);
}

So I propose to replace "must" with "should" in the description of MESH_PRIMITIVES_UNEQUAL_JOINTS_COUNT (while keeping error code) and to reduce its severity to Warning or Information.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good 👍

if (warnOnMultipleExtensions && extensionMaps.length > 1) {
context.addIssue(SemanticError.multipleExtensions,
args: [null, extensionMaps.keys]);
}
Copy link
Contributor

@donmccurdy donmccurdy Mar 6, 2018

Choose a reason for hiding this comment

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

Not sure I understand this one ... why is having multiple extensions reported as an issue? Your test case makes sense — multiple material extensions may interact in undefined ways — but reporting an issue when KHR_materials_unlit is used with KHR_draco_mesh_compression makes less sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

This warning is effective only when several extensions are defined for the same object.
For now, it's enabled only for material objects:

final extensions =
getExtensions(map, Material, context, warnOnMultipleExtensions: true);

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

@emackey
Copy link
Member

emackey commented Mar 6, 2018

ISSUES.md and CHANGELOG.md look good to me.

Currently having trouble with pub get. Running dartsdk-2.0.0-dev.32.0-windows-x64 and I cleared my old pub cache. Running pub get prints only one message:

Resolving dependencies...

But this returns immediately, without any delay that would normally be associated with attempting to download a number of packages from a server. No further messages are printed, errors or otherwise.

I'm pretty sure this is a problem with my local machine or local network. I tried switching back to master and the old dartsdk, which used to work, and it's not getting any packages or errors now. I may need to try this from a different machine on another network.

@lexaknyazev
Copy link
Member Author

@emackey
You could try pub get -v.

@emackey
Copy link
Member

emackey commented Mar 6, 2018

pub get -v

Thanks. Corporate cert went bad, fixed now.

Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

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

Looks good to me. Also I tested this in VSCode with a mismatched Joint/Weight sample, seems to work fine.

@lexaknyazev lexaknyazev changed the title [WIP] 2.0.0-dev.1.7 2.0.0-dev.1.7 Mar 7, 2018
@lexaknyazev lexaknyazev merged commit 42d1a52 into master Mar 7, 2018
@lexaknyazev lexaknyazev deleted the 2.0.0-dev.1.7 branch March 7, 2018 18:14
@emackey
Copy link
Member

emackey commented Mar 7, 2018

This is published to npm now.

https://www.npmjs.com/package/gltf-validator

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

Successfully merging this pull request may close these issues.

4 participants