Compute InstancedMesh' InstanceBuffers at render time based on distance from camera.

[iwanders]

Hi again!

So, I ran into a rendering problem with InstancedMesh displaying cubes with transparency. The rendering order of this was based on the order in which the instances are assigned, and this results in some undesirable visualisations.

I was working on an effect to 'deconstruct' a bunch of cubes into smaller cubes that fall to the ground and then fade out. The problem is that some of these instances were being drawn in front of instances they should have been behind: vehicle_before. The tracks should clearly be hidden behind the instances that make up the green body of the vehicle.

Searching in the issues, I think #154 relates to this problem. Currently all objects are sorted by bounding box to determine the rendering order, but for the individual instances in the InstancedMesh this is not done.

In 2759377 I tried to modify the instanced shapes example to show the problem, it's on branch transparent-instancing-ordering-example, the before screenshot of the example:

It's not super clear, but the green cube (behind the front cube) is drawn later than the front cube and it all ends up looking off.

With the changes proposed in this branch that example ends up looking like this:

The same vehicle screenshot from earlier ends up looking correct: vehicle_after

This works by:

• Storing the instances object as a private variable.
• Removed instance_buffers from InstancedMesh.
• Moved creation of the instance_buffers to a private function that also takes the camera.
• In this method we iterate through the instances based on their position origin. It won't be perfect, neither will this solve rendering if there's two InstancedMesh objects that overlap. While not perfect, it's an improvement over the current state.
• In draw, render_with_material and render_with_postmaterial we first calculate the instance buffers based on our camera position, allowing us to order the instances.

[asny]

That's definitely a problem, but one I deliberately chose not to solve at the moment. Did you test the performance impact of your solution? To me it sounds very expensive to create and fill all of the instance buffers each frame.

[iwanders]

That's definitely a problem, but one I deliberately chose not to solve at the moment.

Ah, I see, after seeing #154 I expected it was just a matter of "didn't get to" yet.

Did you test the performance impact of your solution? To me it sounds very expensive to create and fill all of the instance buffers each frame.

I did not, but I was already calling set_instances each frame, so for me there is no performance difference with this change. I'm happy to produce some numbers, but wouldn't really know how to approach benchmarking this. I can see this be a big difference in other use cases where there's a lot more instances and a lot less changes. In the instanced_shapes we also call set_instances each frame, but I didn't give the change in performance much thought as I assumed the update to always happen each frame.

I first tried to solve it where I generate the instances. But the problem is that I didn't have the camera pose there. Then, as I was changing my signatures to propagate the camera I realised that this would break down if I ever needed two camera's. So then tried to fix it 'in the proper place' (from the view of my usage at least).

It's easy to make this behaviour optional and opt-in though, we could set a boolean called order_by_camera_depth or something. And if that's false, we set the buffers from set_instances, without re-ordering (current approach). If it is true we recompute the order whenever we do the render and have the camera position? What's your opinion on that approach? I think it could give us an easy way to support both situations?

[asny]

Ah, I see, after seeing #154 I expected it was just a matter of "didn't get to" yet.

Yeah, well it's also a matter of limited time, but I also didn't have a good solution and I also thought that having transparent instances where a bit of a special case. I actually think your solution is the right solution - just want to make that absolutely clear - my main concern is performance. But it might not be a big issue, I don't know, it's always good to be challenged about your assumptions. The problem is that it can be difficult to test, because it also depends on hardware. Also, performance wouldn't be a problem if OpenGL supported that you could set the order of instances, like an index buffer but for instances instead of vertices. However, I don't think it does, at least I couldn't find it, it only support to specify the number of instances that you want to draw.

Anyway, to render correctly when the instances are transparent, we need to sort the instances back to front and that need to happen every frame (in the render methods), no doubt about that. Of course it only needs to happen if something has changed, but if not, then you shouldn't call the render methods. Also, they only need to be sorted if they are transparent, if not, everything is working. Finally, if you have a transparent object in between the transparent instances, then you'll first need to render some instances, then the object and then some more instances - maybe we can live with that special case not working, just wanted to mention that it will not work in all cases anyway.

So I think the right solution in most cases is to either make the objects not transparent (transparency always cause troubles in a rasteriser) or use individual objects instead of instances (the RenderTarget::render method sorts the individual objects). I know it's kind of a lame answer, but at least consider if there's another solution that using transparency. If there's no other solution or you want to live with the performance problems, I think we need to use your solution (we can add it no matter what you choose of course, someone else probably needs it 🙂 ), but only if the instanced mesh is transparent. So I like to avoid flags if at all possible, it's not a nice interface, and instead derive it from the data. Luckily we already have the data, because we can get the MaterialType from the Material and check whether it's transparent. If it is transparent, then we can do the sorting, otherwise, just use a fixed set of buffers. It might not be as easy to do as it sounds, but hope your up for the challenge? 💪

[iwanders]

Good morning, thanks for the detailed response.

I don't think it does, at least I couldn't find it

I also searched, and likewise couldn't find anything for it.

Anyway, to render correctly when the instances are transparent, we need to sort the instances back to front and that need to happen every frame (in the render methods), no doubt about that. [... ] Also, they only need to be sorted if they are transparent, if not, everything is working. Finally, if you have a transparent object in between the transparent instances, then you'll first need to render some instances, then the object and then some more instances - maybe we can live with that special case not working, just wanted to mention that it will not work in all cases anyway. [...] So I think the right solution in most cases is to either make the objects not transparent (transparency always cause troubles in a rasteriser)

Yeah, I'm slowly starting to understand that transparency in renderers is a 'thing'... I never really gave it any thought. Reading a bit more it seems like a very complex topic. I think we're fine with 'good enough' and just treating the elements in this InstancedMesh, otherwise the complexity of the problem because significant.

Of course it only needs to happen if something has changed, but if not, then you shouldn't call the render methods.

I may be misunderstanding this sentence. But aren't there valid cases where nothing has changed in this InstancedMesh, but I still call the render method to get the instances (that themselves are unchanged) redrawn in a scene where other things changed?

or use individual objects instead of instances (the RenderTarget::render method sorts the individual objects).

That wasn't performant enough, I'm trying to make an effect where my large cube falls apart into many smaller cubes and rendering each small cube as individual objects would be too slow. I had to switch to the InstancedMesh even for my collection of large cubes (making up my vehicles), and now I'm effectively using the InstancedMesh as a particlesystem. Which is also why I need opacity, I want to be able to fade them out.

If there's no other solution or you want to live with the performance problems, I think we need to use your solution (we can add it no matter what you choose of course, someone else probably needs it 🙂 ), but only if the instanced mesh is transparent. So I like to avoid flags if at all possible, it's not a nice interface, and instead derive it from the data. Luckily we already have the data, because we can get the MaterialType from the Material and check whether it's transparent. If it is transparent, then we can do the sorting, otherwise, just use a fixed set of buffers. It might not be as easy to do as it sounds, but hope your up for the challenge?

Ok, I think that's a good approach. Some thoughts on this approach:

• Overall; in render_with_material and render_with_post_material, we check material_type() to determine if it is transparent. If it is transparent, we determine the ordering based on the camera position and render that.
• The render_with_* methods have an immutable reference, so they cannot write to the InstancedMesh. That means that for opaque materials to avoid having to update the instance buffers, they should already exist, so always create them at the time of set_instances, but there we don't have the material, so that means that for transparent materials we would end up making them twice per (set_instances & render_with*) call. Is that ok? We also can't store the instance buffers to re-use them if the camera (and instances) doesn't change. (Unless we do some interior mutability thing... in which case we can probably also avoid the duplicated creation of the buffers). I personally think this is fine, given that - as you said - this change only affects transparent instances really, if they are more expensive to render reasonably correct that's still better than them currently being really incorrect ;)
• Regarding the rendering order the page on transparency sorting states opaque objects first, do we want to treat instances where a==255 differently and sort those to the beginning? Or should we just go with the answer we get from the material and then sort by depth (based on instance origin) farthest-first?

[asny]

Good morning, thanks for the detailed response.

No problem, thanks for your involvement 💪

Yeah, I'm slowly starting to understand that transparency in renderers is a 'thing'...

Yes, it is definitely a thing, which is not a thing if you do ray tracing - but that's another story 🙂 I agree that we should just make it as good as possible, and then be open about the limitations with regards to transparency.

I may be misunderstanding this sentence. But aren't there valid cases where nothing has changed in this InstancedMesh, but I still call the render method to get the instances (that themselves are unchanged) redrawn in a scene where other things changed?

Sorry, wasn't really clear. Just wanted to mention that we shouldn't worry about whether or not the InstancedMesh actually needs to be rendered, that's left to the user. And yes, we wouldn't be able to judge if it needs to be rendered, because something else in the scene could have changed, even though camera and the InstancedMesh haven't changed. Hope it makes more sense, otherwise, don't worry about it, it was a side note 🙂

That wasn't performant enough, I'm trying to make an effect where my large cube falls apart into many smaller cubes and rendering each small cube as individual objects would be too slow. I had to switch to the InstancedMesh even for my collection of large cubes (making up my vehicles), and now I'm effectively using the InstancedMesh as a particlesystem. Which is also why I need opacity, I want to be able to fade them out.

That's fair, just wanted to add that as a possibility.

Overall; in render_with_material and render_with_post_material, we check material_type() to determine if it is transparent. If it is transparent, we determine the ordering based on the camera position and render that.

Agree 👍 However, I didn't think about that the instances are also transparent if the alpha value of the per-instance colour is not 255. So the instances must be sorted if the material is transparent or if the colour alpha value is less than 255.

The render_with_* methods have an immutable reference, so they cannot write to the InstancedMesh. That means that for opaque materials to avoid having to update the instance buffers, they should already exist, so always create them at the time of set_instances, but there we don't have the material, so that means that for transparent materials we would end up making them twice per (set_instances & render_with*) call. Is that ok? We also can't store the instance buffers to re-use them if the camera (and instances) doesn't change. (Unless we do some interior mutability thing... in which case we can probably also avoid the duplicated creation of the buffers). I personally think this is fine, given that - as you said - this change only affects transparent instances really, if they are more expensive to render reasonably correct that's still better than them currently being really incorrect ;)

I also agree with you here; either have two sets of buffers, one for rendering it with opaque materials and one that's created whenever it needs to be rendered with a transparent material, or use a RwLock to mutate the buffers if it's being rendered with a transparent material. I would choose the RwLock, it's better for performance and it's completely incapsulated. What do you think?

Regarding the rendering order the page on transparency sorting states opaque objects first, do we want to treat instances where a==255 differently and sort those to the beginning? Or should we just go with the answer we get from the material and then sort by depth (based on instance origin) farthest-first?

Yeah, you are right. If the material is transparent, then all of them should be treated as transparent and sorted back to front. If the material is not transparent, but the per-instance colour has an alpha value less than 255, then you should sort the instances such that the instances with a==255 is rendered first, and then the instances with a < 255 sorted back to front. It's getting complex, but hope it makes sense 🙂

A few additional notes:

I actually think the fireworks example has the same problem, I had used the Add blend mode previously but changed it recently to default blend mode because it didn't seem to make a difference. However, I now think I did it with Add because then it didn't matter how the instances where sorted. Anyway, you might be able to edit that example to make it visible if you're interested.

I've started slowly working on animation support, that should hopefully make it possible to avoid having to call set_instances each frame 🙂

[iwanders]

So, this is something I ran into several times now, I always build with --release, because otherwise everything is slow, but that hides this assertion. Then it fails elsewhere, without a nice convenient print that's like; your color vector isn't equal size. What's the actual cost of doing this assertion and panicking with a nice message?

Though, now that I think of it it's probably possible to compile with optimisations and debug assertions. Yes, may be worth documenting somewhere?

[iwanders]

Need an expert opinion here, I don't know enough about what a PostMaterial is, or how it is used to determine whether we should default to transparent or opaque.

[asny]

A PostMaterial is similar to a material but applied one at a time after everything else has been rendered, because it needs the intermediate rendered image. For example a blur effect applied on top of the rendered image. Since it's applied one at a time, I thought I didn't need the material type, but I didn't think of this case. So that needs to be added, but I can do that later, just assume it's not transparent for now.

[iwanders]

Ok, I spent some time coding this all up. And - indeed - it's gotten a bit complex 😬

So I started the evening by making a new test example. It can be found here and currently lives on branch instanced-transparency-test. I'm happy to add it to this branch if that makes testing easier. The current output of that example in master is:

Left row is transparent, right row is opaque materials, all equal size cubes sized (1.0, 1.0, 0.1), all plane offsets 1 unit apart (away from camera) and drawn in the following order:

Color::new(0, 255, 0, 255),   // green, closest, should obscure everything.
Color::new(255, 0, 255, 255), // purple, behind green, second opaque plane.
Color::new(255, 0, 0, 128),   // Red, third plane, should be behind two opaques, blend in front of blue.
Color::new(0, 0, 255, 128), // Furthest, blue layer.

It is surprising to me that the transparent materials (but a==255) are affected by light differently from the opaque materials. Another effect of transparency being so special?

While implementing I ran into the set_instance_count function, which made things more complex. I think we an only use that if we have an opaque material with non-opaque instances, so that's currently how it works. Because that's the only situation (and probably the most common) where we don't use the reordering, I hope I got this right. Tested this with opaque_meshes.set_instance_count(3) while writing this comment: I didn't, the right column in the test is an opaque material, but it has transparent instances, which means it gets re-ordered, but the set_instance_count operated as if there is no re-ordering. 8daf071 fixes that, we should only iterate up to instance_count with the indices, and then those get sorted and written to the buffer, I think that way it all works out.

However, I didn't think about that the instances are also transparent if the alpha value of the per-instance colour is not 255. So the instances must be sorted if the material is transparent or if the colour alpha value is less than 255.

Well, I for one have difficulty thinking of a material that is not transparent, but the instances then still have an alpha that is non 255 and a useful value?

Yeah, you are right. If the material is transparent, then all of them should be treated as transparent and sorted back to front. If the material is not transparent, but the per-instance colour has an alpha value less than 255, then you should sort the instances such that the instances with a==255 is rendered first, and then the instances with a < 255 sorted back to front. It's getting complex, but hope it makes sense

I tried to stick as close as possible to these few sentences in the implementation, I split out the decision whether we should update and what the drawing order then should be. The ordering itself happens here, especially the OpaqueFirstTransparentBackToFront case is a bit daunting.

I don't really know how to test that OpaqueFirstTransparentBackToFront case visually, I don't know what a non-transparent material with transparency in the colors could represent, so that logic is implemented, but I can't visually confirm that.

For the other case, the transparent material the new example, with this branch at dc74034 gives:

and rotating the camera to the other side also looks as I would expect. I modified the example a bit while writing up this comment to also have the non-transparent material and non-transparent instances in it to test the instance count method there.

[asny]

Again, sorry for the late reply.

I think this is getting a bit too complex, luckily I realised that I was wrong. The instances only need to be sorted if the material is transparent, it doesn't matter if the alpha value is less than 255, since that is not used if the blend state doesn't use it. Therefore it should be sorted if the material is transparent and not sorted if the material is not transparent. Should make things a bit more simple. Also, I would really like to avoid InstanceBufferState, just save a HashMap<String, InstanceBuffer> as is done today and update it if the material is transparent. Also, you can remove the instance_count, I think it overcomplicates things for a very specific use case that can be achieved by setting the instances instead.

I hopefully have time to look more into it soon, but at least you have some more input 🙂

[iwanders]

I think this is getting a bit too complex

Haha, yes... I was thinking it was getting complicated when I was coding it, fun challenge though. Anyways, glad you realised it could be simpler, it's always nice to be able to remove complexity.

Therefore it should be sorted if the material is transparent and not sorted if the material is not transparent. Should make things a bit more simple.

Yep, 'opaque material & transparent instances' was the scenario that made things the most complicated.

Also, I would really like to avoid InstanceBufferState, just save a HashMap<String, InstanceBuffer> as is done today and update it if the material is transparent.

I didn't see how to avoid re-creating the buffers each frame if the camera stays the same without a state (or at least, camera position), but given that the complexity is now much lower I just went with a (HashMap<String, InstanceBuffer>, Vec3) in the RwLock, it's not just the hashmap, but I think that extra complexity is a small price to pay for not having to update the buffers each frame if the camera didn't move? Unless there's some other way?

Also, you can remove the instance_count, I think it overcomplicates things for a very specific use case that can be achieved by setting the instances instead.

I think I actually managed to accommodate it neatly in the updated code. Since we only have to worry about this for transparent materials, we can just check it whenever we check the camera pose and we can check it by looking at any length of InstanceBuffer directly. If that changes from the current instance count, we need to re-create the buffers.

Simplified code is pushed, I did keep the unit test for the ordering function. I also noticed draw was a private method, so I could just pass in the already acquired InstanceBuffer references instead of looking them up there. Number of lines changed is still significant, but overall the code and proposed change should be much easier to understand.

[asny]

Super nice 💪 I'll be happy to merge it as it is, so let me know if you don't think this is fun anymore or want to do other stuff 😄

Haha, yes... I was thinking it was getting complicated when I was coding it, fun challenge though. Anyways, glad you realised it could be simpler, it's always nice to be able to remove complexity.

Glad you feel that way, I like to remove code as well, but not all like to remove what they have spend time doing, which is understandable 🙂

I didn't see how to avoid re-creating the buffers each frame if the camera stays the same without a state (or at least, camera position), but given that the complexity is now much lower I just went with a (HashMap<String, InstanceBuffer>, Vec3) in the RwLock, it's not just the hashmap, but I think that extra complexity is a small price to pay for not having to update the buffers each frame if the camera didn't move? Unless there's some other way?

I think your solution is perfectly all right, so we can keep it like that, no problem 🙂 If you want to spend more time on it, I had an idea of storing the instance indices instead of the camera position, and then in each render call check if it's sorted (do something like this). If it's not sorted, then sort the indices and update the stored set of indices and update the buffers. What do you think?

I think I actually managed to accommodate it neatly in the updated code. Since we only have to worry about this for transparent materials, we can just check it whenever we check the camera pose and we can check it by looking at any length of InstanceBuffer directly. If that changes from the current instance count, we need to re-create the buffers.

Great, keep it if it's not complicating things, but if it significantly complicates things, I'm ok with removing it 🙂

Simplified code is pushed, I did keep the unit test for the ordering function. I also noticed draw was a private method, so I could just pass in the already acquired InstanceBuffer references instead of looking them up there. Number of lines changed is still significant, but overall the code and proposed change should be much easier to understand.

Great with a unit test 👍 You could also sort and update the instance buffers in the draw method instead of in both the render methods, but it's a minor improvement.

Do you also want to add the example you created for this?

Finally, I was wondering why rendering instances was so much more performant so I did a profiling and turns out that all of my string handling and hashmap lookups is the bottleneck 😬 So the problem is CPU side and can be fixed. Will look into it as soon as I have time 💪

[iwanders]

I had an idea of storing the instance indices instead of the camera position, and then in each render call check if it's sorted (do something like this). If it's not sorted, then sort the indices and update the stored set of indices and update the buffers. What do you think?

I'm not sure actually, comparing camera position is trivial, it is always three float comparisons, regardless of how many instances we have. Checking if the indices are ordered requires calculating their distances and then comparing them, so that's linear with the number of instances.

I don't know much about graphics 'things', but you said that the transfer of the buffers is expensive, so I can see value in checking if we have to do the transfer at all by seeing if the index ordering has changed, but I would do so in addition to the camera check, not instead of, just because checking if it is ordered is much more expensive than checking just the camera pose. I'm fine adding it, but I'm just not too sure if we need this optimisation just yet, like I didn't notice it when my buffers updated every frame.

Thinking about this some more - at the cost of high bookkeeping - you could also check the largest consecutive non-ordered section in the buffer and only rewrite that. Because it is very unlikely the ordering of objects far away from the camera changes if the camera moves just a little bit. Technically, that can all live in one place in the buffer object and that can do a delta update. We definitely don't need this, just musing.

You could also sort and update the instance buffers in the draw method instead of in both the render methods, but it's a minor improvement.

Yeah, that's better, that moves that action into one location, I'll give draw a boolean to indicate material transparency and remove the instance_buffers argument later.

Do you also want to add the example you created for this?

Yeah sure, I currently have the transparency_draw_order one, which I've used for screenshots in this PR; it has a row of cubes with transparency and a row of opaque ones (and the - to be removed - opaque material, transparent instances). Is that what you were thinking? Or do you want an example that shows the limitations of the current approach? Like two intersecting instances, which will render correctly only from one side, or two overlapping InstancedMesh objects where the region where they overlap problems can occur?

[iwanders]

I was wondering why rendering instances was so much more performant so I did a profiling and turns out that all of my string handling and hashmap lookups is the bottleneck

Did you use anything specific to Rust for this? Or just the usual tooling one would use for any program on linux? Familiar with the latter, so just wondering if there's anything worth knowing specifically for rust.

You could also sort and update the instance buffers in the draw method instead of in both the render methods, but it's a minor improvement.

Heh, we missed that we also pass the instance_buffers to the vertex_shader_source here, so if we only update the buffers in the draw method that's too late. So I'm not sure if this is possible.

In 1f0d504, I added an example. But obviously there's no 'without reordering' anymore, so to make it more insightful, I added a situation where the current approach always breaks:

Also changed the camera to orbit, such that it is easy to rotate around it.

With that, unless you have any other outstanding remarks you'd like to see addressed, I think this is good to go in.

[asny]

I'm not sure actually, comparing camera position is trivial, it is always three float comparisons, regardless of how many instances we have. Checking if the indices are ordered requires calculating their distances and then comparing them, so that's linear with the number of instances.

Good point 👍

Yeah sure, I currently have the transparency_draw_order one, which I've used for screenshots in this PR; it has a row of cubes with transparency and a row of opaque ones (and the - to be removed - opaque material, transparent instances). Is that what you were thinking?

Yes 🥳

Did you use anything specific to Rust for this? Or just the usual tooling one would use for any program on linux? Familiar with the latter, so just wondering if there's anything worth knowing specifically for rust.

I'm on mac, so I used the cpu profiler in "Instruments", it's a general profiler, so not specific to Rust 🙂

Heh, we missed that we also pass the instance_buffers to the vertex_shader_source here, so if we only update the buffers in the draw method that's too late. So I'm not sure if this is possible.

Arh, yeah, I didn't think of that.

In 1f0d504, I added an example. But obviously there's no 'without reordering' anymore, so to make it more insightful, I added a situation where the current approach always breaks:

Perfect 👍

With that, unless you have any other outstanding remarks you'd like to see addressed, I think this is good to go in.

I have nothing else, thanks for your contribution 🚀 🥳 🎉