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

BasisTextureLoader: Next steps #16524

Closed
10 of 12 tasks
donmccurdy opened this issue May 22, 2019 · 62 comments
Closed
10 of 12 tasks

BasisTextureLoader: Next steps #16524

donmccurdy opened this issue May 22, 2019 · 62 comments

Comments

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 22, 2019

Initial support for .basis textures added in #16522. This issue tracks remaining cleanup and planned enhancements. I'm not working on these yet, and will update this issue when I begin, so PRs are welcome in the meantime:

  • Clean up remaining TODOs in the code
  • Apply eslint fixes
  • Add documentation
  • Add example
  • Add setMaxWorkers() method
  • Support mipmaps
  • Fix mipmap support in iOS (working at BasisTextureLoader: Refactor #21131)
  • Return texture synchronously from load() (added at BasisTextureLoader: Refactor #21131)
  • Support alpha
  • Add ES module
  • Recompile Basis transcoder with bootstrapping suggested by @austinEng
  • Support user-configurable transcode output format
@takahirox
Copy link
Collaborator

I haven't looked into the code in detail yet but could we synchronously return texture from load as other texture loaders do?

I think it's good for the consistency. And if we don't synchronously return texture user needs to call material.needsUpdate = true in callback (if already started animation loop).

var material = new XXXMaterial();
textureLoader.load(..., function (texture) {
  material.map = texture;
   // .map is from null to non-null.
   // User needs to call material.needsUpdate = true here if already started animation loop
   // because whether material.map is null or not affects the final shader code.
  material.needsUpdate = true;
});

@donmccurdy
Copy link
Collaborator Author

I haven't checked yet to confirm that works with THREE.CompressedTexture, but if so I agree that would be best. 👍

@donmccurdy
Copy link
Collaborator Author

Other cleanup: the properties assigned to the texture are a little arbitrary (left over from Basis demo), like flipY=false. And there's an unused startTime variable in the worker.

@takahirox
Copy link
Collaborator

takahirox commented May 23, 2019

If I'm right mipmaps doesn't seem to be supported. Transcoder doesn't support? Or loader hasn't just implemented mipmaps support yet?

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented May 23, 2019

A .basis file can contain multiple mipmap levels, yes. I think the transcoder supports it already, but I haven't tested that. BasisTextureLoader doesn't support it yet.

We should update to the new/smaller version of the transcoder, too: BinomialLLC/basis_universal#7 Done.

@austinEng
Copy link

Yup, the transcoder should support it. You pass the mip level as the levelIndex to transcodeImage.
https://github.com/BinomialLLC/basis_universal/blob/master/webgl/transcoder/basis_wrappers.cpp#L197

@takahirox
Copy link
Collaborator

Thanks for your explanations.

And if there is any other functionalities the loader hasn't supported yet, but transcoder does, you are aware of, please add to TODO list. We can help implementation.

@takahirox
Copy link
Collaborator

Made an example. #16553 It may be too simple, please feel free to enhance/replace later if it's merged.

@donmccurdy
Copy link
Collaborator Author

On other features the transcoder has but THREE.BasisTextureLoader does not, a key difference is that the transcoder can output many additional formats:

https://github.com/mrdoob/three.js/blob/dev/examples/js/loaders/BasisTextureLoader.js#L264-L273

I don't honestly know when to use all of these. I expect that the user will sometimes want control of that, and in other cases a loader (e.g. GLTFLoader) will make the decision based on the purpose of the texture. For example, it might choose a different compressed format for material.map (base color) than for material.aoMap (ambient occlusion).

The most obvious way to support this would be to add an alternative to detectSupport( renderer ):

// Let loader decide, based on device capabilities:
loader.detectSupport( renderer );

// Or, choose a particular format:
loader.setFormat( THREE.BasisTextureLoader.BASIS_FORMAT.cTFBC4 );

This has a potential problem – if I'm loading multiple textures, we might not want to transcode them all to the same format. We could create multiple loaders, but then it's harder to reuse existing Web Workers (which is important). We could pass a format into the load() method, but then it's not backward compatible with TextureLoader. I guess the best thing might be to ensure that doing this...

loader.setFormat( THREE.BasisTextureLoader.BASIS_FORMAT.cTFBC4 );
var fooTex = loader.load( 'foo.basis' );

loader.setFormat( THREE.BasisTextureLoader.BASIS_FORMAT.cTFBC1 );
var barTex = loader.load( 'bar.basis' );

... will always apply the right format to each texture, even though decoding is asynchronous.

@donmccurdy
Copy link
Collaborator Author

@donmccurdy
Copy link
Collaborator Author

Another note, just to keep this tracked somewhere: the JS wrapper in examples/js/libs/basis contains a minor change from the version in the Basis repository. The first declaration (var Module) is replaced with just Module to enable the lazy initialization used in a Web Worker. This can probably be done differently, either by compiling the transcoder with different flags or via BinomialLLC/basis_universal#21.

@zeux
Copy link
Contributor

zeux commented Jun 2, 2019

Should BasisTextureLoader work in tandem with glTF? I've tried manually encoding textures to .basis format and adding BasisTextureLoader as a loader like this:

var basisLoader = new THREE.BasisTextureLoader();
basisLoader.setTranscoderPath( 'basis/' );
basisLoader.detectSupport( renderer );

THREE.Loader.Handlers.add( /\.basis$/i, basisLoader );

But the textures are not rendered properly, and the console has the following output:

[.WebGL-000002B68031CEF0]RENDER WARNING: texture bound to texture unit 1 is not renderable. It maybe non-power-of-2 and have incompatible texture filtering.

@arpu
Copy link

arpu commented Jun 3, 2019

same Problem as @zeux describe

@zeux
Copy link
Contributor

zeux commented Jun 3, 2019

I debugged this and this happens because:

  1. glTF loader usually sets mag filtering to LinearMipMapLinearFilter
  2. BasisTextureLoader doesn't support mipmaps
  3. webGL requires mip chains to be complete :-/

@donmccurdy
Copy link
Collaborator Author

@zeux Not officially supported – the core glTF spec allows only JPEG and PNG textures. But an extension for Basis (via KTX2 wrapper) is the mechanism by which we plan to add compressed texture support to the format. See KhronosGroup/glTF#1612 for draft extensions (only the first two are relevant here) and feel free to add feedback.

That said, the lack of mipmap support in BasisTextureLoader is purely because we haven’t gotten to it yet. The transcoder itself should support that as far as I know.

@zeux
Copy link
Contributor

zeux commented Jun 3, 2019

Submitted PR to fix mipmap support - with this as long as you convert textures with -mipmap option, glTF loader just works as long as you add the loader as listed above, at least on desktop. I wasn't able to get it to run on mobile (iPhone or Android), but threejs.org example with the spinning cube doesn't work on iPhone either so that might be a separate issue.

@donmccurdy
Copy link
Collaborator Author

I wasn't able to get it to run on mobile (iPhone or Android)

From the Basis docs –

For example, on iOS, you can only use square power of 2 texture dimensions for PVRTC1, and there's nothing Basis can do for you today that works around this limitation. (We will be supporting the ability to trancode smaller non-pow2 textures into larger power of 2 PVRTC1 textures soon.)

We've used a 512x768 texture in this demo, and should probably replace it with something that fits that constraint.

@zeux
Copy link
Contributor

zeux commented Jun 4, 2019

Ok - that makes sense. FWIW the Android phone I was testing on has a whole host of issues with multiple WebGL demos, not just Basis texture related - a different Android phone runs just fine. So yeah it's probably just the power-of-two restriction that's problematic on iOS.

@donmccurdy
Copy link
Collaborator Author

Various updates to BasisTextureLoader coming in #16675.

@donmccurdy
Copy link
Collaborator Author

We should probably also think through how to support alpha... the Basis documentation goes into some detail on the options (https://github.com/BinomialLLC/basis_universal/#how-to-use-the-system) but for some devices this involves multiple transcoding outputs:

ETC1 only devices/API's: Transcode to two ETC1 textures and sample them in a shader. You can either use one ETC1 texture that's twice as high, or two separate ETC1 textures.

So far the API matches TextureLoader, which would need to change (or have an alternative API) to support returning multiple transcoded outputs from a single .basis texture.

@donmccurdy
Copy link
Collaborator Author

Changing to a square power-of-two texture in #16686 fixes the demo on iOS, but mipmaps aren't working:

INVALID_VALUE: compressedTexImage2D: length of ArrayBufferView is not correct for dimensions.

Do we need to do anything particular for mipmaps with PVRTC?

@zeux
Copy link
Contributor

zeux commented Jun 5, 2019

Is that happening for one of the last few mips?

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jun 17, 2019

@Makio64 Something like that! Most threejs loaders are not stateful (a single loader could be working on multiple things in parallel) so maybe:

const [ map ]           = loader.loadRGB( 'foo.basis', { ...options } );
const [ map, alphaMap ] = loader.loadRGBA( 'bar.basis', { ...options } );

^In both cases, but especially alpha, I think there may be enough different ways to do things to require some configuration on the method.

Or we could just go asynchronous on the new methods, instead:

loader.loadRGBA( 'bar.basis', { ...options }, ( map, alphaMap ) => {
  // ...
} );

@Makio64
Copy link
Contributor

Makio64 commented Jun 17, 2019

The asynchronous solution look more easy to implement with the worker and align with the others threejs's loaders.

@mozg4D
Copy link

mozg4D commented Jun 19, 2019

I'm only able to load textures with resolution 768 by 512 or 384 by 256 etc. any other resolution fails to load with Three.js and BasisTextureLoader with warning:
"texture bound to texture unit 0 is not renderable. It maybe non-power-of-2 and have incompatible texture filtering."

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jun 19, 2019

@mozg4D please see the Basis documentation, in particular power-of-two textures are required in iOS. If the documentation doesn't match the behavior you're seeing, please file a new bug. It's also possible that the texture filtering selected is incompatible, in which case we'd still need a demo and a new bug would be helpful. Thanks!

@Ben-Mack
Copy link

Ben-Mack commented Jul 5, 2019

@donmccurdy Will Basis alpha support be released soon?

@nh2
Copy link
Contributor

nh2 commented Sep 27, 2019

I think I found a bug: On iOS only, there's a weird "texture glowing" effect across texture-geometry edges: #17597

Has anybody else encountered this?

@shrekshao
Copy link
Contributor

I think on iOS it will likely use PVRTC. Does this happen in an earlier version (r108)?
Maybe you could try file an issue in https://github.com/BinomialLLC/basis_universal?

@nh2
Copy link
Contributor

nh2 commented Sep 27, 2019

Does this happen in an earlier version (r108)?

The bug I filed is for r108.

Maybe you could try file an issue in https://github.com/BinomialLLC/basis_universal?

Was just in the process of doing so: BinomialLLC/basis_universal#78

@richgel999
Copy link

I think I found a bug: On iOS only, there's a weird "texture glowing" effect across texture-geometry edges: #17597

I replied on the basis_universal github. We'll grab the texture and see what's happening. Most likely, it's an artifact related to not setting the wrap/clamp addressing transcode flag correctly, or an artifact caused by our real-time PVRTC1 encoder. If either is the problem, there are usually workarounds. We can also increase PVRTC1 quality, at the cost of more transcode CPU time/memory.

@nh2
Copy link
Contributor

nh2 commented Sep 28, 2019

I've posted an update on BinomialLLC/basis_universal#78 (comment) -- screenshots included there.

It shows the wrapping-around problem with a non-spinning cube.

@nh2
Copy link
Contributor

nh2 commented Sep 28, 2019

I found another bug (but most likely unrelated): #17546 (comment)

@nh2
Copy link
Contributor

nh2 commented Oct 1, 2019

I found another bug (but most likely unrelated): #17546 (comment)

Fixed in #17622.

@supermoos
Copy link

In terms of mipmaps, is there a way to properly load basis texture files (referenced inside a .gltf file) without having to embed the mipmaps in the .basis file?

I can get it to load proplery when I generate the .basis file with -mipmap, however this adds a lot of filesize to the .basis file - but when I generate a .basis file without -mipmap option, it simply shows as black in the browser with threejs.

@JohannesDeml
Copy link
Contributor

In terms of mipmaps, is there a way to properly load basis texture files (referenced inside a .gltf file) without having to embed the mipmaps in the .basis file?

I can get it to load proplery when I generate the .basis file with -mipmap, however this adds a lot of filesize to the .basis file - but when I generate a .basis file without -mipmap option, it simply shows as black in the browser with threejs.

For now, you can disable mipmaps to make the texture show:
https://discourse.threejs.org/t/compressed-texture-workflow-gltf-basis/10039/12?u=johannesdeml
JohannesDeml@909d9cc

That of course is no support for mipmaps, but if your goal is to just show textures, that might be an easy solution for you.

@supermoos
Copy link

That doesn't seem to be working anymore, and it looks like the BasisTextureLoader now also has similar code (setting the min/magFilter = LinearFilter) when only 1 mipmap is detected (

texture.minFilter = mipmaps.length === 1 ? THREE.LinearFilter : THREE.LinearMipmapLinearFilter;
texture.magFilter = THREE.LinearFilter;
) - which is what basisu without the -mipmap option generates. However it's still black.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Feb 10, 2020

Can you share the file? I was doing this as recently as last week, although it was Basis in a .ktx2 container rather than .basis...

And just to confirm – you're aware that you can't generate these mipmaps at runtime, and would have to make do with the interpolation constraints involved?

@supermoos
Copy link

supermoos commented Feb 10, 2020

Sure, and thanks for taking a look!

body_green.basis.zip

And just to confirm – you're aware that you can't generate these mipmaps at runtime, and would have to make do with the interpolation constraints involved?

Yes, I figured that out too, a bit of a shame since much of the the gain I saw in lower filesize of .basis compared to a compressed jpg is then lost - I know the GPU memory gain is still there, but I was mostly focused on download/transfer size of the texture.

@polarathene
Copy link

And just to confirm – you're aware that you can't generate these mipmaps at runtime, and would have to make do with the interpolation constraints involved?

Is this always the case when using basis instead of jpg/png?

What about a use-case of packing 6 textures as facelist for a cube map into a single basis file as the format supports/mentions this on the README. Is PMREM generator useless here and the basis file should have mipmaps for each texture generated?

What about providing this packed texture data for use as a cubemap in ThreeJS? Normally you'd pass args for each individual texture? (I haven't looked into this texture loader support yet to see if a basis file with multiple textures is possible) An alternative perhaps is via KTX2 which might more suitable for providing a facelist packed basis file to be used as a cubemap?

One final question about this usage as you were discussing handling of alpha, these textures could be encoded as RGBE or RGBM(ThreeJS has support for M7 and M16). I haven't investigated how well those compress with basis, but they can have similar issues as normal maps have been known to. The alpha channel data is fairly important to have supported there, I'm not sure what compressed texture format they'll be transcoded to, some may produce pretty poor results as the linked article below addresses.

ARM wrote an article about issues with ASTC and RGBM for example. Unity engine as the article mentions uses RGBM5, which should in most cases cover a suitable HDR range, the lower multiplier presumably would produce less compression quality issues than a higher multiplier constant if the data fits within the range limits.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 22, 2020

Sorry, I don't know the answer to these questions. You may have better luck on the Basis Universal or KTX GitHub repositories. I do know that KTX2 is designed to support cubemaps with Basis payloads.

@polarathene
Copy link

I raised them here as they seemed relevant cases for using the ThreeJS support with being discussed here.

Basis can store all 6 textures for a cubemap in a single basis file. KTX2 with Basis support can cater to better supporting cubemaps with multiple textures but a single file afaik.

ThreeJS being able to handle it in future is unclear. Perhaps 6 different basis files need to be provided, or gain KTX2 with basis loader support.

RGBM5 would need a separate PR, it'd just be RGBM7 or RGBM16 that currently exist, or a variant that takes a uniform to adjust the multiplier value. The main important part there is the alpha needs to be handled appropriately to compress right, so being able to have some control on that as discussed earlier in this issue is another example of where such would be useful, similar to Normal Maps and their Linear/non-colour encoding, which may also have the format split into two possible textures(from single colour RGB for X, and alpha for Y in the basis file) depending on compression support.

@donmccurdy
Copy link
Collaborator Author

Three.js currently supports Basis Universal. A new Basis format, UASTC, was released just a few weeks ago. I do not think Basis supported cubemaps in a single file prior to that. Pull requests would be welcome.

A KTX2 loader is in progress, with #18490.

@polarathene
Copy link

Older versions of the README for Basis(prior to UASTC, but still present in current README) shows mention of the multiple texture packing into single basis file feature:

Basis files support non-uniform texture arrays, so cubemaps, volume textures, texture arrays, mipmap levels, video sequences, or arbitrary texture "tiles" can be stored in a single file. The compressor is able to exploit color and pattern correlations across the entire file, so multiple images with mipmaps can be stored very efficiently in a single file.

Pull requests would be welcome.

I would not know where to start, nor do I have the spare time at present. Perhaps once this issue progresses along to completion and the KTX2 loader is ready, the compressed/packed cubemap support could be addressed. If I have spare time by then, I'd be happy to try contribute a PR :)

@igghera
Copy link

igghera commented Jun 22, 2020

Hi all, I've got an intermittent issue on iOS (aawww maaaan!)
Basically the texture sometimes loads fine, sometimes it doesn't appear.
No error in the console, load progress is 100%, so definitely not a network issue.
Tested both on the official example (https://threejs.org/examples/?q=basis#webgl_loader_texture_basis) and on my own project. On my iPhone X, on Safari, the texture sometimes appears and sometimes it doesn't.
On iPhone 6, Safari, it never appears.
All the others look fine.
What could it be?

[EDIT] just got the same issue on Safari on Mac OS, still intermittent

@donmccurdy
Copy link
Collaborator Author

@igghera Sounds like this may be a bug, especially if it's happening on the official example. Do you mind opening a new issue for this? Thanks!

@Jarred-Sumner
Copy link

Jarred-Sumner commented Dec 10, 2020

How might one go about adding support for 2D array textures in BasisTextureLoader? I looked into this a little but I'm not really sure how to proceed.

Changing the transcode function to loop over the images from the count returned by basisFile.getNumImages() seems straightforward, but it looks like there are three things to address following that:

  1. compressedTexImage3D does not exist in THREE.WebGLState and would need to pass an option for gl.TEXTURE_2D_ARRAY per WebGLRenderingContext.compressedTexImage3D
  2. Basis' API returns individual mipmaps per image index, but you need a single large texture blob to bind for sampler2DArray?
  3. CompressedTexture would need a way to associate mipmaps per image index. For a 200 deep array image and 6 mipmap levels, that'd be 1200 mipmap entries, so probably mipmaps in CompressedTexture becomes a strided array. Or is there a way that WebGL abstracts that detail away?

Then, for video textures, it'd probably need a different implementation. Rather than transcoding in batch at once, you'd keep the basis file handle open and have some way to request the next frame.

@donmccurdy
Copy link
Collaborator Author

See #19676 for more on compressed array textures.

@donmccurdy
Copy link
Collaborator Author

I don't think there is anything left to do from the checklist in this issue, after #21131. Rather than keep this issue open indefinitely let's close it and start new issues for future bugs or requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests