pex-gl / pex-context

Modern WebGL state wrapper for PEX: allocate GPU resources (textures, buffers), setup state pipelines and passes, and combine them into commands.
http://pex-gl.github.io/pex-context/
MIT License
160 stars 12 forks source link

Add warning if attribute with divisor present but no instance count provided #126

Closed vorg closed 3 months ago

vorg commented 1 year ago

Due to how we detect instancing it is possible to have a geometry with offsets and no instances count and no instanced shader present yet still attempt to render instances resulting in no draw call const instanced = Object.values(cmd.attributes || cmd.vertexArray.attributes).some(attrib => attrib.divisor); https://github.com/pex-gl/pex-context/blame/v3/index.js#L1072

https://github.com/pex-gl/pex-context/blame/v3/index.js#L1094.

Maybe a warning would be good?

dmnsgn commented 1 year ago

Would checking && (Number.isFinite(cmd.instances) && cmd.instances > 1) be too much automagic?

vorg commented 1 year ago

I think it should be the default. Not sure what's the point of checking divisor if no instanced count is provided. Your version using .some() is just refactor of my attribute by attribute checks. Happy to go ahead with

const instanced = Number.isFinite(cmd.instances) && cmd.instances >= 1)

vorg commented 1 year ago

Turns out the point is that when we animating sth there might be zero instances of a cube which is a valid draw call. Checking for Number.isFinite(cmd.instances) introduced a bug where vertex array for instanced attributes were being enabled but they had no data and drawElements call was causing Context Loss.

Note: simply not enabling those attributes with divisor if instances==0 doesn't seem to work. I would expect one cube to be drawn in the center of the scene but nothing happens probably becase pex-renderer already assigned shader with instanced attributres???

dmnsgn commented 1 year ago

Reverted in 266287217840a46adf19df79746455c943bd2c54 for 3.0.0-alpha.5