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

GLTFLoader: Set RGBFormat for jpg with no mimeType. #21892

Merged
merged 2 commits into from
May 27, 2021
Merged

GLTFLoader: Set RGBFormat for jpg with no mimeType. #21892

merged 2 commits into from
May 27, 2021

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented May 26, 2021

Description

It seems like it can be the case that an image source has uri but not mimeType.
TextureLoader automatically sets RGBFormat based on the url but ImageBitmapLoader doesn't do that.

An example of a model is DamagedHelmet:

"images" : [
{
"uri" : "Default_albedo.jpg"
},
{
"uri" : "Default_metalRoughness.jpg"
},
{
"uri" : "Default_emissive.jpg"
},
{
"uri" : "Default_AO.jpg"
},
{
"uri" : "Default_normal.jpg"
}
],

@donmccurdy @takahirox does this make sense to you?

@donmccurdy
Copy link
Collaborator

Yes, glTF only requires the mimeType field when the image is embedded as binary data (i.e. in a .glb). One other case that might hit a similar issue would be a Data URI, so perhaps the regexp should check for the data:image/jpeg; prefix at the beginning of the URI as well? Here's an example (although it uses PNG) ...

https://github.com/KhronosGroup/glTF-Sample-Models/blob/a334d137c0e86fa9455a0d6037d3aa8859af7abd/2.0/BoxTextured/glTF-Embedded/BoxTextured.gltf#L140-L144

@mrdoob
Copy link
Owner Author

mrdoob commented May 27, 2021

@donmccurdy Done! 👍

@mrdoob mrdoob added this to the r129 milestone May 27, 2021
@mrdoob mrdoob merged commit 370504b into dev May 27, 2021
@mrdoob mrdoob deleted the gltf branch May 27, 2021 01:02
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.

2 participants