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

Second Sync Update #86

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Second Sync Update #86

wants to merge 4 commits into from

Conversation

DRx3D
Copy link
Contributor

@DRx3D DRx3D commented Dec 21, 2023

The models added here need some assistance - mostly descriptions. This is meant to be applied after PR #85 and after the 'issues' tag has been removed.

The new models in this PR are:

  1. ClearcoatRing
  2. ClearcoatSphere
  3. SheenHighHeel
  4. SuzanneMorphSparse
  5. TransmissionSuzanne

In addition other models may have had various updates to READMEs to improve the formatting.

@DRx3D
Copy link
Contributor Author

DRx3D commented Dec 22, 2023

All of the models listed in this PR all need descriptions. Most were supplied by UX3D. Tagging a few UX3D people to attract their attention. The model description can be added as a comment and I will get it into the system. You can also create a PR against the forked branch if it is a substantial change.

@UX3D-becher , @UX3D-haertl , @UX3D-kanzler

Model READMEs (from the PR source)

  1. ClearcoatRing
  2. ClearcoatSphere
  3. SheenHighHeel
  4. TransmissionSuzanne

@DRx3D
Copy link
Contributor Author

DRx3D commented Dec 22, 2023

@javagl, @lexaknyazev : The SuzanneMorphSparse model needs a description. Can either of you provide something simple for this?

@DRx3D DRx3D mentioned this pull request Dec 22, 2023
@javagl
Copy link
Contributor

javagl commented Dec 22, 2023

Maybe this was already supposed to be reviewed, and I missed it, but I'd suggest to not include SuzanneMorphSparse in its current form.

  • It claims to use KHR_materials_unlit and KHR_texture_transform but uses neither of these
  • It contains some undocumented/useless extras
  • It contains a node that has rotation/translation/scale set, but all with their default values
  • Most importantly: It does not contain any morph animation.

I don't know where the screenshot GIF comes from, but none of the common viewers that I tried shows anything "animated" there. (That small white thing that appears (in the GIF) in the lower left when the eyes are closed are a minor detail at this point...)

I know that this means that #61 will remain open, but... that's what it is for now.

@lexaknyazev
Copy link
Member

It contains some undocumented/useless extras

Those are actually spec-suggested, see the note at the end of the Morph Targets spec section. Agreed with all other points.

There's also inconsistent indentation in SuzanneMorphSparse/metadata.json.

@DRx3D DRx3D marked this pull request as draft January 5, 2024 21:01
@DRx3D
Copy link
Contributor Author

DRx3D commented Jan 5, 2024

Changed to Draft so comments can be resolved without merging.

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