pex-gl / pex-renderer

Physically based renderer (PBR) and scene graph for PEX.
https://pex-gl.github.io/pex-renderer/examples/index.html
MIT License
237 stars 16 forks source link

Call getPexMaterialTexture per texture not per material #306

Closed vorg closed 1 year ago

vorg commented 2 years ago

For buster drone there is only 3 materials and 10 textures but in the current code:

In old version of the loader we pre-processed textures, materials etc in first pass before traversing tree to avoid duplicated work like above. Not sure when this has changed.

vorg commented 2 years ago

I'm also getting TypeError: Failed to execute 'texImage2D' on 'WebGLRenderingContext': parameter 9 is not of type 'ArrayBufferView'. in Chrome despite last param being ImageBitmap with valid width and height. But only on the last texture.

dmnsgn commented 2 years ago

getPexMaterialTexture is called for every material texture (in handleMaterial) but texture2D creation is cached in _tex so no problem of loading textures twice:

https://github.com/pex-gl/pex-renderer/blob/bc51a9f385b841ebb46916d4dd4848778898e41a/loaders/glTF.js#L242

ctx.texture2D is called 39 times in Chrome but 10 times in Safari (sth sth ImageBitmap?)

Cannot reproduce with a webgl2 context: I get the same amount in chrome and safari, 10.

vorg commented 2 years ago

Cannot reproduce with a webgl2 context: I get the same amount in chrome and safari, 10.

Most likely because https://github.com/pex-gl/pex-context/issues/120 (ImageBitmap bug) and while it is attempted 40x and if it fails then _tex is not set.

Now calling 40x handleMaterial instead of 3x can be either bug or feature. It definitely adds up for complex gltf scene with a benefit that each entity will have their own material component as they should be unique anyway. It is then up to renderer to notice that 40 materials need only 3 shaders via flags.

dmnsgn commented 1 year ago

ImageBitmap bug fixed in pex-context v3: https://github.com/pex-gl/pex-context/pull/117