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

Develop/gltf sample viewer enhancements #342

Merged
merged 19 commits into from
Dec 3, 2023

Conversation

UX3D-haertl
Copy link
Contributor

This PR adds models which were previously used for the sample viewer

@donmccurdy
Copy link
Contributor

Seeing a number of 4K textures containing just a solid color here – would it be possible to swap those for a factor?

@cx20
Copy link
Contributor

cx20 commented Mar 16, 2022

Some models seem to have cases where the file name differs from the folder name. Following convention, it is more consistent to match file and folder names. Since SheenChair already exists, it would be better to place it in a different folder.

Below are the proposed changes.

before after
ClearcoatRing/glTF/PBR - Metallic Roughness Alpha-blend.gltf ClearcoatRing/glTF/ClearcoatRing.gltf
SheenChair/glTF/gltf_sheen_damask.gltf SheenDamask/glTF/SheenDamask.gltf

@UX3D-haertl
Copy link
Contributor Author

I transformed the base color and emissive textures to factors and renamed to assets as suggested

@cx20
Copy link
Contributor

cx20 commented Mar 17, 2022

@UX3D-haertl Thank you for the improvement.
I think the readme and screenshot also need to be added.

Model Nmae README.md screenshot
ClearcoatRing ⚠️ Link Error
ClearcoatSphere
SheenDamask
SheenHighHeel
TransmissionSuzanne

@cx20
Copy link
Contributor

cx20 commented Mar 19, 2022

I experimentally tried the ClearcoatRing model with gltf-test.
However, the display result of each library seems to be different.
I could not determine if the problem with the see-through back was a model problem or a library problem.

image image
image image

@cx20
Copy link
Contributor

cx20 commented Mar 19, 2022

I also checked SheenDamask. This model seems to look quite different from library to library when viewed from an angle.

image image
image image

@donmccurdy
Copy link
Contributor

However, the display result of each library [for ClearcoatRing] seems to be different.

The ClearcoatRing materials are all using "alphaMode": "BLEND" — I think that would be a mistake in this case.

@cx20
Copy link
Contributor

cx20 commented Mar 19, 2022

@donmccurdy Thank you for your advice.
I tried removing the alphaMode property of the material locally to test it out.

https://github.com/KhronosGroup/glTF-Sample-Models/blob/develop/gltf-sample-viewer-enhancements/2.0/ClearcoatRing/glTF/ClearcoatRing.gltf#L244

Results appear to be good.

image

@DRx3D
Copy link
Contributor

DRx3D commented May 10, 2023

The automated validation failed. We would like to get this merged in the next week if everything is clean and good to go.

@DRx3D
Copy link
Contributor

DRx3D commented May 11, 2023

@echadwick-wayfair , @emackey : Please make a final review of the models

@DRx3D
Copy link
Contributor

DRx3D commented Nov 17, 2023

@UX3D-haertl : Except for the conflict with model-index.json, is this PR ready to go?

@UX3D-haertl
Copy link
Contributor Author

Yes it is ready from my side

@DRx3D DRx3D merged commit 16b6dde into master Dec 3, 2023
2 checks passed
@DRx3D DRx3D deleted the develop/gltf-sample-viewer-enhancements branch December 3, 2023 03:50
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.

5 participants