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

Move GetTextureDataAsync from inspector to core #14312

Merged
merged 7 commits into from Sep 27, 2023

Conversation

kircher1
Copy link
Contributor

This is a refactor that will eventually enable a serializer like GLTFExporter to use the now core-based GetTextureDataAsync in order to serialize textures that are not directly retrievable as RGBA e.g., GPU-compressed formats.

For more discussion on that point, see #12257. The part that is missing to ultimately solve this is some sort of function or BaseTexture-property that can tell whether the texture uses a GPU compressed format. Once that exists, then it should be easy to do something like this... from glTFMaterialExporter.ts:

image

@bjsplat
Copy link
Collaborator

bjsplat commented Sep 15, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Sep 15, 2023

@Popov72
Copy link
Contributor

Popov72 commented Sep 18, 2023

This code copies an existing texture (compressed or not) to a non-compressed RGBA texture, which is not what we want in the exporter (the exporter is already able to export compressed textures, but it exports the result of the expansion to RGBA 8 bits to a .png file, not the original compressed data).

[EDIT]
My bad, I really thought the exporter was able to export a compressed texture, though in non-compressed format!
But I tested it in a simple PG and it didn't work...
So, I guess there's a need for the PR after all.
[/EDIT]

To be able to export compressed textures in their compressed form, we would need to be able to retrieve the compressed data, which is not possible because readPixels does not work in this case:

https://playground.babylonjs.com/#E6WY1N

You will get this error in the console log:

[.WebGL-00002DB408078E00] GL_INVALID_FRAMEBUFFER_OPERATION: Framebuffer is incomplete: Attachment is not renderable.

And the buffer will be all zero.

Even if that worked, you would only get the transcoded data that was suitable for the target GPU when the texture has been created, whereas you would need the non-transcoded data for the export.

Regarding the function itself, I'm not sure we should move it into the misc tools, as it seems quite specific to the inspector...

cc @sebavan for his opinion.

@RaananW
Copy link
Member

RaananW commented Sep 18, 2023

which is not what we want in the exporter

If the decision is whether or not the texture is exported, this seems to be a viable solution

My EDIT - just saw YOUR edit :-). This can be ignored.

packages/dev/core/src/Misc/textureTools.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Misc/textureTools.ts Outdated Show resolved Hide resolved
Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

LGTM. I agree with @Popov72 regarding the inspector-only code, which should stay in the inspector

packages/dev/core/src/Misc/textureTools.ts Outdated Show resolved Hide resolved
@kircher1
Copy link
Contributor Author

Yeah, just to clarify, as you found the glTF exporter does not handle GPU compressed textures now. There is a related issue here: #12257

So the idea with this helper function would be to render out a copy of the compressed texture as RGBA. (I have tried a local version of this and it does indeed work. The 'compressed' texture gets serialized as a png)

packages/dev/core/src/Misc/textureTools.ts Outdated Show resolved Hide resolved
packages/dev/core/src/Misc/textureTools.ts Outdated Show resolved Hide resolved
@sebavan
Copy link
Member

sebavan commented Sep 26, 2023

@bghgary and @Popov72 can I let you do a last review ?

packages/dev/core/src/Misc/textureTools.ts Outdated Show resolved Hide resolved
packages/dev/inspector/src/textureHelper.ts Outdated Show resolved Hide resolved
@bghgary
Copy link
Contributor

bghgary commented Sep 26, 2023

I think this is ready to merge.

@RaananW RaananW merged commit cd75618 into BabylonJS:master Sep 27, 2023
10 checks passed
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

6 participants