material.id is missing and leads to cache issues

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

234 stars 16 forks source link

material.id is missing and leads to cache issues #320

Closed dmnsgn closed 10 months ago

dmnsgn commented 1 year ago

New components don't increment material ids: https://github.com/pex-gl/pex-renderer/blob/41759af9e1652e7c050df58a0821acd8022c4b80/components/material.js#L158-L177

As a result, pipeline cache might use the wrong material as the following:

https://github.com/pex-gl/pex-renderer/blob/41759af9e1652e7c050df58a0821acd8022c4b80/systems/renderer/standard.js#L424

translates to undefined_sharedProgramId_sharedGeometryPrimitive in getGeometryPipeline.

vorg commented 1 year ago

Well that's why program.id is used. As program is cached by flags and those should contain all the required features. Do you have a test case?

dmnsgn commented 1 year ago

Test case:

add log where pipeline creation happens in getGeometryPipeline: https://github.com/pex-gl/pex-renderer/blob/41759af9e1652e7c050df58a0821acd8022c4b80/systems/renderer/standard.js#L426-L428
go to gltf example TextureSettingsTest
see it failed doubleSided test because it doesn't go through pipeline creation and reuse another material (I assume)
add id: Material_${MaterialID++} to material component returned value, see test passing

vorg commented 1 year ago

I think that id was omitted on purpose to avoid material explosion. For now i would double check if we cache all the flags eg culling and long term find better approach than dirty flag.

vorg commented 10 months ago

Should be fixed in v4 so closing.