Vertex error and no colors on thin instances after upgrade

Bugs
21

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!

1 Like
22

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:

23

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.

1 Like
24

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
25

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
26

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
27

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

Thanks for your help and have a good one Blake.

1 Like
28

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
29

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
30

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
31

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.

1 Like
32

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

33

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

34

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
35

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

36

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

2 Likes
37
3 Likes
38

Hey folks!

I just got back to this, been a rough couple of weeks :expressionless:.

I basically reverted all the changes I had done to get around the various bugs, upgraded from 5.0.0-alpha.60 to 5.7.0, tested and it automagically worked. Of course, I know who are the automagicians: @Cedric @Blake @Evgeni_Popov @Evgeni_Popov @sebavan – incredible team effort :slight_smile:.

Thanks a lot for the support here, and the hardcore devotion to backward compatibility. This is one of the reasons I picked BJS 3 years ago and it keeps paying back :pray:.

Thanks again and kudos!

5 Likes
39

Always love reading this kind of message and the main reason we are all pumped to continue :slight_smile:

1 Like