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

Hide the model that has > 4 jointweights per vertex #469

Merged
merged 2 commits into from Nov 14, 2018

Conversation

stevk
Copy link
Contributor

@stevk stevk commented Nov 13, 2018

None of the clients that have setup previews work with this model. Hiding it for now, since the spec doesn't have an explicit direction on how this case should be handled.

  • Comment out model code
  • Comment out figure for this skin in the readme
  • Delete the generated model

@stevk stevk self-assigned this Nov 13, 2018
@stevk stevk requested review from kcoley and bghgary November 13, 2018 20:57

properties.Add(new Property(PropertyName.Description, "`skinF`."));
}, (model) => { model.Camera = distantCamera; }),
//CreateModel((properties, animations, nodes) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to put a comment before this saying why this is commented out.

@donmccurdy
Copy link

The spec isn't clear on this case yet...

I think the spec is fairly clear — most engines don't support >4, and the spec mentions that. Any particular details to add?

@bghgary
Copy link
Collaborator

bghgary commented Nov 14, 2018

I think the spec is fairly clear — most engines don't support >4, and the spec mentions that. Any particular details to add?

Do you mean this?

Client implementations must support at least two UV texture coordinate sets, one vertex color, and one joints/weights set.

I couldn't find anywhere that says most engines don't support >4. Currently, all of the test assets we have are positive tests meaning they are supposed to work everywhere. I don't think we should include a test with more than one set of joints/weights unless we somehow mark them.

@bghgary
Copy link
Collaborator

bghgary commented Nov 14, 2018

@stevk mentioned to me that the spec has this:

https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#skinned-mesh-attributes

The number of joints that influence one vertex is limited to 4, …

The issue with this is that we are saying we need a test model here: KhronosGroup/glTF#1403

The >4 issue is here: KhronosGroup/glTF#1381

The question for me is whether a model with >4 joints/weights per vertex is a positive test or a negative test.

@stevk stevk merged commit 768254f into KhronosGroup:master Nov 14, 2018
@stevk stevk deleted the hidemodel branch November 14, 2018 19:25
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.

None yet

3 participants