Vertex error and no colors on thin instances after upgrade

Bugs
1

Hello,

I’m trying to upgrade BJS on my app from alpha.60 to rc.4 but i’m having a new issue. My thin instances colors are not working anymore (it’s black instead of per instance colors). Also there are vertex errors when I call thinInstanceSetAttributeAt.

Offending line [348] in vertex code: vColor=instanceColor;

VERTEX SHADER ERROR: 0:348: 'instanceColor' : undeclared identifier
ERROR: 0:348: '=' : dimension mismatch
ERROR: 0:348: 'assign' : cannot convert from 'const highp float' to 'out highp 4-component vector of float'

Cc @Cedric as I’m seeing you worked on something related to that in Babylon.js/what's new.md at preview · BabylonJS/Babylon.js · GitHub. Hopefully it will ring a bell :wink: .

Sorry that this is in closed source code and I can’t seem to be able to repro in PG.

Thanks in advance.

2

I’m taking a look. Thank you for reporting.

3

@tibotiber

Can you modify this PR so you can repro the shader compilation bug :

4

Thanks @Cedric. So, I’m not using any custom shader. It’s purely color attributes on thin instances. PG wise, it’s more similar to this one:

I’ve been pocking around it but it looks pretty much like my app from what I see and I can’t repro the black colour.

Sorry that this leaves very little for debugging opportunity :thinking:. This bug really appeared out of an upgrade of BJS (nothing else changed) and the only reference to colors and thin instances I could find in “what’s new” was this piece on vertex shaders.

Any idea?

5

This issue is still on my radar. It might have been fixed with recent bugfix. Can you please try to repro with latest rc?

6

Thanks @Cedric. I’ve just tried with rc.13 on 3 browsers and still get the same error. The offending line has move by one up, that’s the only difference.

Happy to assist in debugging but I’m honestly not sure where to start. :thinking:

7

Thank you for testing @tibotiber

It will be difficult to fix it without a playground.

8

@tibotiber could you cut your app to the bare minimal repro and share on github ?

9

It’s not trivial but will try that, and maybe crack having it happen in a PG :crossed_fingers:. Thanks for being ok to check something that’s not PG. I will also try to pinpoint exactly which version introduced the error.

1 Like
10

Hi again, so I can finally confirm the error appeared in beta.6.:skull_and_crossbones:, everything was fine in beta.5.:rainbow:. Finding this out reminded me of maths classes about dichotomy, lol. I’ll be digging some more to understand what may be happening and potentially isolate a repro. And I’m obviously joking with the version smileys, thought to make Friday error reports more fun :stuck_out_tongue:.

Since I’m here, big round of applause on the release, you folks rock and deserve a good celebration. Looking forward to the announcement.

1 Like
11

so this might narrow it down to this Native Thin Instances · BabylonJS/Babylon.js@99259f9 · GitHub

What material are you using ?

12

Yes, that’s what I’ve been looking at as well, although I don’t fully grasp what’s happening there. My material is a PBRMetallicRoughnessMaterial.

Here are some of the (simplified) manipulations on the material and instances I’m running in case it helps:

export const makeMaterial = ({
  scene,
  name,
  materialParameters: {
    baseColor,
    metallicRatio = 0,
    roughnessRatio = 1,
    alphaRatio = 1
  },
  onError = () => {}
}) => {
  const material = new PBRMetallicRoughnessMaterial(name, scene)
  material.baseColor = Color3.FromHexString(baseColor)
  material.metallic = metallicRatio
  material.roughness = roughnessRatio
  material.alpha = alphaRatio
  material.onError = onError
  return material
}

export const instantiateMesh = ({
  mesh,
  instances,
  scalingFn,
  colorFn
}) => {
  // prepare root mesh for instancing
  mesh.alwaysSelectAsActiveMesh = true
  if (mesh.isPickable === true) {
    mesh.thinInstanceEnablePicking = true
  }
  // prepare custom buffer for instances color
  // https://doc.babylonjs.com/divingDeeper/mesh/copies/instances#custom-buffers
  let bufferColors = new Float32Array(4 * instances.length)
  // create thin instances
  let bufferMatrices = new Float32Array(16 * instances.length)
  let index = 0
  const addInstanceToBuffers = ({
    instance,
    __instanceIndex
  }) => {
    // position mesh
    const { position, rotation, scaling } = computeMeshPosition({
      instance,
      useQuaternion: true
    })
    const instanceSpecificScaling = scaling.scale(scalingFn(instance))
    const matrix = Matrix.Compose(instanceSpecificScaling, rotation, position)
    matrix.copyToArray(bufferMatrices, index * 16)
    // instance base color in RGBA (with multi-materials suport)
    const color = Color3.FromHexString(colorFn(instance))
    bufferColors[index * 4 + 0] = color.r
    bufferColors[index * 4 + 1] = color.g
    bufferColors[index * 4 + 2] = color.b
    bufferColors[index * 4 + 3] = 1 // we don't support alpha on instance due to render order issues, see babylon forum
    // next
    index++
  }
  instances.forEach((instance, __instanceIndex) =>
    addInstanceToBuffers({ instance, __instanceIndex })
  )
  const staticMatrices = false
  const staticColors = false
  mesh.thinInstanceSetBuffer('matrix', bufferMatrices, 16, staticMatrices)
  mesh.thinInstanceSetBuffer('color', bufferColors, 4, staticColors)
}

I had thought maybe I needed to name the buffer “instanceColor” instead of “color” (looking at the changes in the commit) but it’s doesn’t solve the issue and the PG still works with “color”. Is it sure the shader is reading from the “color” buffer?

I’m still trying to isolate a repro.

13

Heya, I was able to make a repro for this I think. Here’s a simple PG using the thin instance color attribute where the thin instances are black but they should be red like in this PG that sets the color attribute on the geometry instead.

It looks like all that’s missing is the part to push the color attribute, which I just copied from StandardMaterial and it fixes this repro locally. :+1:

3 Likes
14

You seem to have nailed it @Blake! For verification, I took this PG which works, swapped the material to PBRMetallicRoughnessMaterial and now it’s black. https://playground.babylonjs.com/#RC2IAH#47.

I feel silly I didn’t think of trying another material :man_facepalming:. Admittedly, I haven’t touched materials in over a year in my app so it’s really not top of mind. That’s a good learning for me.

15

PR for black PBR instances : Fix instance color forwarding for PBR Instances by CedricGuillemet · Pull Request #12333 · BabylonJS/Babylon.js · GitHub

1 Like
16

Glad it has finally been fixed, twice :slight_smile:

1 Like
17

Sometimes it takes the power of a double fix.

That’s gonna give away my age… :sweat_smile:

1 Like
18

Captain Tsubasa just proves out you are at the best age :wink:

2 Likes
19

Hi @Cedric and @Blake,

I just realised there is another odd behaviour related to this. After your PRs, the instances are coloured properly, however if you try to update a single instance with thinInstanceSetAttributeAt, there is no effect. It would if you use thinInstanceRegisterAttribute to register the color but then thinInstanceSetBuffer is not working. The docs and behaviour so far was that thinInstanceRegisterAttribute would not be required if you use thinInstanceSetBuffer.

I’ve updated the PG to demonstrate this issue:

In the code, the first box you hover should turn red, it doesn’t. Uncomment line 59 and it does but the buffer is moot. There is also a timeout to color one of the boxes for quick testing (it’s there because I was worried by hover logic was wrong).

Any idea what might be happening?

Thanks!

20

Heya, if you use pass “instanceColor” (or VertexBuffer.ColorInstanceKind) instead of “colors” when you call thinInstanceSetAttributeAt at line 136 then it works…

Also I submitted a little PR to make thinInstanceSetAttributeAt check for the old value and convert it to the new one automatically, so then it will still work if you pass “colors”. :slight_smile:

2 Likes