Vertex error and no colors on thin instances after upgrade

Sometimes it takes the power of a double fix.

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

1 Like

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

2 Likes

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!

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

Ah, I had tried that but I probably screwed up somewhere else when I did :sweat_smile:. Thanks for the help and PR for backward compatibility. So i’m up to the next bug (please bear with me :grimacing:). Now if you try to clone the instanced mesh in that PG, you get a shader error. I don’t really “see” anything wrong on the scene but the shader is screaming something that seems related to the color to instanceColor change. I thought it might be worth reporting.

PG below and interesting line is 107.

Thanks again!

Hmm, after copying over the instancesCount then it works:

clone._thinInstanceDataStorage.instancesCount = source._thinInstanceDataStorage.instancesCount;

For example here’s your PG with this fix and also here’s a simpler PG to show the issue (if you comment out line 45 then you can see how it isn’t working without setting the instancesCount).

EDIT: I started a new thread for this issue of needing to set the internal count directly since it turned out to be unrelated. :slight_smile:

Hi Blake,

So this is not the bug I’m seeing actually. I’ve modified your simpler PG to reflect the bug:

It’s basically the same but with PBR material on the sphere, and I’m cloning the instanced mesh into a non instances mesh. I think that when switching from instanced to non-instanced, some color / instanceColor reference in the shader must be missing. Maybe the material / shader should preserve both values so we can switch between instanced or not without issue? (I honestly don’t understand the internals)

Notes about the PG:

  1. if you clone from instanced to instanced (uncomment line 54), there is no error.
  2. if you remove the PBR material (set false line 25), there is no error.

I hope this helps. Happy to help further but i’m not sure where to look.
Cheers.

Oops, I didn’t realize that you were trying to clone the mesh that has thin instances and get a mesh without thin instances. I don’t think that will work because they share the same vertices and buffers… I’m pretty sure what you need for that is unique geometry.

For example you can clone the mesh and call makeGeometryUnique() on the clone before you start with any thin instances and then clone that mesh whenever you want a clone without thin instances. Maybe others will have a better way but that’s the only way I’ve ever found to do it… :slight_smile:

1 Like

Thanks for the super quick reply.

That’s strange because I’ve been cloning like this (without making the geometry unique) without any issue until this change to color / instanceColor. I appreciate the solution but it’s not ideal as I potentially have a large number of meshes to clone “just in case” they’re interacted with. At the moment, if a mesh is selected to make changes, then I clone it on the fly to put gizmos on it, etc. I’m worried about the impact of having to keep a disabled clone for each mesh in my scene.

So far the only precautions I’ve had to take when cloning were to call clone.thinInstanceCount = 0 and clone.useVertexColors = false.

Interestingly, the code actually works and the scene is behaving as expected right now. Just there is this shader error that gets printed. Is there a chance we might find a simple fix to maintain the behaviour I’ve relied on without having this error printed? It seems to boil down to the color / instanceColor variables being kept in sync, but I’m not sure exactly where to do that.

Thanks again for the help :slight_smile:

1 Like

Ooh I never thought to try it that way, sounds good if don’t want to use vertex colors on it anyway… I’m not sure what would be needed to keep that behavior when using PBR thou, better wait for @Cedric and @Evgeni_Popov. :slight_smile:

Have a good one :beers: :sleeping: :zzz:

1 Like

I’d love to… but it’s Monday morning here :sob:.

Thanks for your help and have a good one Blake.

1 Like

This PR will fix the crash:

However, due to how (thin) instance colors are now handled in 5.0, the cloned mesh will not inherit the color of the first thin instance as it happens in 4.2. That’s because vertex colors and instance colors are now two separate things whereas previously vertex colors were reused for instance colors (and so it was not possible to have both vertex and instance colors).

3 Likes

It would be nice (and it’s possible) to mix instance color and vertex colors. I’ll add that to my todo list.

1 Like

That should not be difficult as with your changes we now have separate data. I think we would simply need in the shader to multiply the vertex and instance color when both are used. Currently we have:

#ifdef VERTEXCOLOR
	vColor = color;
#elif INSTANCESCOLOR
	vColor = instanceColor;
#endif

It could be replaced by something like:

#if defined(VERTEXCOLOR) || defined(INSTANCESCOLOR)
    vColor = vec4(1.0);
    #ifdef VERTEXCOLOR
        vColor *= color;
    #endif
    #ifdef INSTANCESCOLOR
        vColor *= instanceColor;
    #endif
#endif

Yes, I was thinking of something like that. Maybe moving that code block into its own macro as it’s duplicated a huge number of times in various shaders.

Also, related to the instance color change, I commented lately on your PR, I don’t know if you saw the comment:

https://github.com/BabylonJS/Babylon.js/pull/12333#discussion_r856880325

Yes I did. I think both issues can be solved in the same PR.

Love the conversation in here :star_struck: and glad to see both backward compatibility improve and capabilities grow. Looking forward to @Evgeni_Popov’s PR being merged and published as well as @Cedric’s one. I’ll keep you posted on my experience once that’s ready for testing. Thanks again!

1 Like

Just for me to better understand, what is the missing part after:

and

went in ?

I guess the shader part ? @Cedric @Evgeni_Popov @Blake is anyone creating the PR for it or should it be in Fix thinInstanceSetAttributeAt to work with "color" kind by BlakeOne · Pull Request #12433 · BabylonJS/Babylon.js · GitHub

Yes, shader part. I’m doing it as I need it for the demo.

2 Likes