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

Add KHR_texture_basisu version of the FlightHelmet#285

Merged
emackey merged 3 commits intoKhronosGroup:masterfrom
bghgary:FlightHelmet-BasisU
May 16, 2023
Merged

Add KHR_texture_basisu version of the FlightHelmet#285
emackey merged 3 commits intoKhronosGroup:masterfrom
bghgary:FlightHelmet-BasisU

Conversation

@bghgary
Copy link
Contributor

@bghgary bghgary commented Jan 6, 2021

UASTC used for non-color textures (normal, ORM). ETC1S used for color textures (baseColor).

TODO:

  • Put information on how this asset was generated, what ktx2 files are UASTC vs ETC1S, etc.
  • Decide if the settings for UASTC/ETC1S is right for this asset

@emackey
Copy link
Member

emackey commented Jan 6, 2021

Since there are no fallback textures, this model must include basisu under extensionsRequired.

We should update any automated tools to include this when fallback textures are not provided.

@bghgary
Copy link
Contributor Author

bghgary commented Jan 6, 2021

Put information on how this asset was generated, what ktx2 files are UASTC vs ETC1S, etc.

Options for this:

  1. Put this in the README.md for the main model.
  2. Put a separate README.md for the BasisU version.

I think 2 makes more sense, but I don't think we've done this before.

@bghgary
Copy link
Contributor Author

bghgary commented Jan 6, 2021

Since there are no fallback textures, this model must include basisu under extensionsRequired.

Good point, though we may want to check with @donmccurdy since this was generated with his tool. I'll update it manually for now.

@donmccurdy
Copy link
Contributor

Will be fixed in glTF-Transform in next release, thanks! 👍🏻

@cx20
Copy link
Contributor

cx20 commented Jan 6, 2021

I experimentally added this model to gltf-test. I think two libraries currently support this extension.

https://github.com/cx20/gltf-test#format-tests
image

@UX3D-haertl
Copy link
Member

It seems like the count of the last three accessors (accessorTangents, accessorNormals, accessorUVs) is too big and needs to be reduced by 1. If one calculates the byte offset of the last element of those accessors, it exceeds the length of the buffer. Apparently the last vertex does not have a tangent, normal and UV.

@donmccurdy
Copy link
Contributor

I'm not seeing anything in the validator report, but will look into it - thanks!

@donmccurdy
Copy link
Contributor

donmccurdy commented Jan 16, 2021

Hm sorry, I'm not able to see what you're finding. These interleaved accessors use the last buffer view, which has 436 vertices and a stride of 48, for a total length of 436 * 48 = 20,928. The last accessor is vec2 floats starting at an offset of 40 bytes, and fits within the last 8 bytes of that stride, so the size seems to work out here.

Perhaps you're trying to allocate count * stride bytes from the starting byte offset of the accessor, which would exceed the buffer view's length? I have made that mistake before.

@UX3D-haertl
Copy link
Member

A yes that was the problem. Thanks for the clarification :)

@elalish
Copy link
Contributor

elalish commented Feb 4, 2021

Is this blocked by anything? It'd be great to have a KTX2 sample model in the repo when the PR goes out. Would also be nice to an official test asset for those of us demonstrating support.

@bghgary
Copy link
Contributor Author

bghgary commented Feb 4, 2021

There are two TODOs in my original comment. Other than that, I don't think so?

@elalish
Copy link
Contributor

elalish commented Feb 4, 2021

Well, I'd rather have something imperfectly optimized than nothing. Shall we just document it and then update it later if we find improvements?

@bghgary
Copy link
Contributor Author

bghgary commented Feb 4, 2021

I'm okay with that. Can we get some opinions on the documentation? #285 (comment)

@emackey
Copy link
Member

emackey commented Feb 4, 2021

I'm okay with that. Can we get some opinions on the documentation? #285 (comment)

I think the model's main README should have a section that explains the BasisU flavor.

@donmccurdy
Copy link
Contributor

FYI: donmccurdy/glTF-Transform#222

@emackey
Copy link
Member

emackey commented Nov 3, 2021

Given the decisions made in #292, the new folder here should be called glTF-KTX-BasisU. I think it's still well worth having a KTX2 version of more than one sample model.

@bghgary bghgary force-pushed the FlightHelmet-BasisU branch from f639371 to 0272057 Compare May 16, 2023 00:34
@bghgary
Copy link
Contributor Author

bghgary commented May 16, 2023

I updated the model using the latest KTX tools and glTF-transform and added a new section to the README. Should be ready to go unless someone has comments.

@bghgary bghgary marked this pull request as ready for review May 16, 2023 00:41
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!

@emackey emackey merged commit 4ca0667 into KhronosGroup:master May 16, 2023
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.

6 participants