repro: https://playground.babylonjs.com/#CP2RN9#214
Switch to 6.49.0 to see the difference. On local dev, I managed to repro in 7.40.3, 7.40.2 does not show the bug. Unless this is a breaking change…
Basically, cloning after setting thin instance fouls up the position of the clone. Did not test rotations/scaling, I presume the transform matrix should be borked somehow?
I’m not sure what we should see in the PG, but we fixed a bug lately wrt to mesh cloning and thin instances:
Note that you can test a PG with different versions of Babylon by adding ?version=XXX in the url:
I cloned the mesh (p1), placed it at (1,0,0). Then added a thin instance to mesh at position(2,2,2). Then I cloned same mesh again (p2), placed it at (2,0,0). In 7.40.2, the clones and thin instance are independent of each other, so I can clone and add thin instances w/o affecting each other.
In 7.40.3, the p2 clone’s position is not at (2,0,0), it seems to be offset from the thin instance’s. I have to place p2 at p2.position.copyFromFloats(0,-2,-2); to get identical output as 7.40.2. Also, if you change rotation of the thin instance with const rotmat = BABYLON.Matrix.RotationY(Math.PI);, the result becomes more obvious.
I tried to build a simple repro according to your description:
White is original mesh, red is p1 and green is p2.
p2 is at (2,0,0) in this PG. The difference with 7.40.2 (https://playground.babylonjs.com/?version=7.40.2#CP2RN9#216) is that thin instances are not lost when cloning, so p2 also has an additional thin instance:
Thin instances of p2 are independent from thin instances of mesh, so you can remove/add thin instances to one mesh without affecting the other one.
Before 7.40.3, when you clone a mesh, the behavior is that: if said mesh has thin instances, the clone does not have thin instances. This is the default. I can have 1 million thin instances and clone mesh twice, visually, I will see 1 million+2, right?
After 7.40.3, when you clone a mesh: if said mesh has thin instances, any subsequent cloning of said mesh will also generate thin instances. This is the new behavior. So, if I have 1 million thin instances and clone mesh twice, suddenly, I will see 3 million?! c.f., https://playground.babylonjs.com/#CP2RN9#217
I just want to get the original behavior (pre-7.40.3) back. What should I do such that when I clone a mesh (that already has thin instances), it does not generate visuals I never asked for/wanted?
Yes, it’s the new behavior, because the old one was a bug: thin instances pertain to a mesh, if you clone a mesh, you should have the thin instances too.
If you don’t want them, you can simply set mesh.thinInstanceCount = 0:
Awesome! Thanks, I got it now!
edit: nvm, I figured it out.
Everything is carried over.
There’s no light in the PG you’ve linked, which is why it’s the emissive color that’s defined, not the diffuse color:
You replied before my edit 
. In hindsight, I think this is a breaking change and the doc should show? I have to hit the sack , will do more testing tom, cheers !
I’ve updated the comment of my PR to indicate it could be a breaking change and how to fix it.
So after sleep and testing on my local dev, I made some observations:
a) mesh.thinInstanceCount = 0; vs mesh.clone(name,newParent?,doNotCloneChildren?,clonePhysicsImpostor?,doNotGenThinInstances?)
I’d prefer for a doNotGenThinInstances flag (default:true) on the clone method to bypass the new code/retain old behavior for some potential savings in mem/cycles. What do you think?
b) If cloning is supposed to retain thin instances, then it should also do the same for normal instances?
c) As I kept testing, this question kept popping up: If clones are supposed to retain thin instances, why not just add thin instances to the buffers of the mesh in the first place?
M1 is the master mesh, C1 is the clone. In Fig1: As per 7.40.3, we are retaining thin instances at creation time which is equivalent to Fig2 where the user should be adding to the buffers of M1 if he/she so require thin instances?
Isn’t 1 set of buffers easier to manage and maintain then passing buffers down to clones? I tot that was the whole point of thin instances in the first place? There must be some burning reason I’m missing…hrm…
Good idea! Would you like to make a PR for it? However, I think we should now support passing an option object to clone, instead of separate flags, to avoid bloating the interface.
Yes, I think it’s debatable (we never had a request for it, that’s probably why regular instances are not cloned).
Sharing thin instance buffers would introduce additional complexities, as we’d have to refcount them. Also, I’m not sure there are many use cases where several different meshes would use exactly the same list of thin instances? If this happens, it’s always possible to share the matrix buffer by calling thinInstanceSetBuffer on both meshes and passing the same buffer.
Thin instances have been introduced as a thin layer to manage instantiation, so we would like to keep this interface as simple as possible and not introduce too much complexity.
Not yet, still testing this breaking change. Found other broken visuals.
Erm, I think there is some miscommunication, I’m not advocating sharing buffers. To be clear, what I meant was the previous 7.40.3 PG: https://playground.babylonjs.com/#CP2RN9#216 is equivalent to this: https://playground.babylonjs.com/?version=7.40.2#CP2RN9#220. I’m not understanding the need for clones to also clone thin instance buffers.
Given my extensive use of clones and thin instances, this ‘bug’ should have been caught when thin instances were first introduced and not after 4/5 yrs. I just don’t understand where the bug is, shrugs…
The problem was that this PG (linked in There is an issue with model cloning) failed because of thin instances not being copied when the mesh is cloned.
We think that copying the thin instances when cloning is the right fix, but I understand your point. Maybe we could update the behavior so that cloning doesn’t copy thin instances by default, and add a parameter to clone to force copying the thin instances. What do you think @sebavan and @Deltakosh? This problem only surfaced 3 weeks ago, so I guess only very few people were impacted by the fact that thin instances weren’t copied.
Sorry, its late here, I’m not sure if I’m seeing stuff correctly. I did a quick look between the old PG: https://playground.babylonjs.com/?version=7.40.2#92Y727#453 and the new:https://playground.babylonjs.com/#92Y727#453 using console.log(container.meshes.length). They both show 193 before and after cloning so the number of meshes match. The glb has a mix of Mesh and InstancedMesh, the obvious difference is that node180 to node191 have scaling of 1. I suspect the user’s model might not have applied transformations during export? But otherwise, I’m not seeing missing thin instances…maybe I’ll take a closer look tom.
The glb file uses the “EXT_mesh_gpu_instancing” extension, which is implemented with thin instances in Babylon. Look at the console log in this modified PG, which dumps meshes with thin instances:
If you browse this PG in 7.40.2 (https://playground.babylonjs.com/?version=7.40.2#92Y727#460), you will see that cloned meshes don’t have thin instances.
Here’s the PR that removes the breaking change:
Ah, so that’s the reason, got it!
Yes, I see it now. I had to switch off 180 meshes and compare PGs. (Idea: a little quality of life fix for the Inspector to allow users to toggle vis/bounding box by clicking & dragging the mouse over the icons instead of having to click each time).
Awesome, tyvm! 