Use mesh instead of immediate for drawing Sprite3D

[clayjohn]

Closes: #20855

Using ImmediateGeometry to draw the Sprite3D made it very slow. On my phone 128 Sprite3Ds ran at 1 FPS.

This PR replaces ImmediateGeometry with a regular old mesh. As a result it needs to store a unique material per Sprite3D so each Sprite can have different textures. ImmediateGeometry got around this because each instance had its own override texture.

With these changes my phone can now have over 1000 Sprite3Ds before FPS drops below 60 FPS. On my desktop, the performance improvement is around 3x.

Note re 4.0: reduz says he wants to improve ImmediateGeometry for Vulkan, so this change may not be worth porting to 4.0. But I will discuss it with him as it should be useful for the low-end backend (GLES2, GLES3 or both?)

[realkotob]

I support this change, it's so essential that I've been using a Mesh instead of Sprite3D for every single billboard in the past 4-5 projects I've worked on. This should be backported to 3.2.x as soon as we have the chance.

Even in 4.0 where ImmediateGeometry will be improved, this might still be relevant depending on the memory-processing tradeoff on different platforms, so we'll have to wait and see. We could end up with 2 different nodes with different implementations (like CPUParticles vs GPUParticles).

[clayjohn]

@asheraryam this PR is targetting 3.2, so no backporting necessary. :)

@akien I'll fix the formatting changes tonight. Clang-format has been acting up on my computer recently.

[lawnjelly]

This looks great! 👍

My only (noob) thing to ask is I'm not that well up on PoolVector access. It seems that each write() call has a bunch of atomic operations to make it thread safe.

	for (int i = 0; i < 4; i++) {
		mesh_normals.write()[i] = normal;
		mesh_tangents.write()[i * 4 + 0] = tangent.normal.x;
		mesh_tangents.write()[i * 4 + 1] = tangent.normal.y;
		mesh_tangents.write()[i * 4 + 2] = tangent.normal.z;
		mesh_tangents.write()[i * 4 + 3] = tangent.d;
		mesh_colors.write()[i] = color;
		mesh_uvs.write()[i] = uvs[i];

Would it make more sense (particularly in the dynamic Sprite3D) to have one write per array, then reuse them for each element?

It probably doesn't make a huge difference, but as much for future reference for best practice this would be useful to know.

[clayjohn]

@lawnjelly I am not sure about the performance impact. I copied this way of doing things from the mesh importer.

I have also seen:

PoolRealArray::Write w = my_array.write();
// Code that writes
w.release();

Being honest, I am not sure which method is better, It appears that the way I have used may not require a release(), but if the other way is better, I am happy to make the change.

[lawnjelly]

I was just looking in the docs here:
https://docs.godotengine.org/en/stable/development/cpp/core_types.html#allocating-memory
And it suggests something like:

PoolVector<int>::Write w = data.write()
w[4] = 22;

It will release automatically when it goes out of scope I think.

So you could wrap the loop like so:

{ // open scope for write
    PoolVector<blah>::Write w = data.write()
    for (int n=0; n<4; n++)
    {
        w[n] = BLAH;
    }
} // close scope for write

(or use release maybe that has the same effect).

I think which one would be better might depend on whether you thought something else would have contention for the same PoolVector at the same time, and whether it's in a tight loop / possibly heavily used. In this case contention would seem unlikely, so personally I'd suspect the single write and reusing might be better (at least for the dynamic case). I'll ask reduz when he's up! 😄

Also one more consideration (that maybe can't occur in this situation, I don't know it well enough) - if at any point there was a separate render thread reading from the PoolVector, then by putting the lock around the whole loop you would ensure that the data was in a 'valid' state. Should a renderer try and read from this during the loop, some elements would contain new data, and some would contain old, resulting in graphical glitches.

It looks great PR though! 👍

[lawnjelly]

If _draw is being called every frame for the AnimatedSprite, it may be worth doing a naive check for whether these states have changed from this side, depending on how involved the checks are for a redundant change in the visual server.

I.e. If texture->get_rid() == last rid, don't set in visual server.

Yes, looking in RasterizerStorageGLES2::material_set_param, it does this:

void RasterizerStorageGLES2::material_set_param(RID p_material, const StringName &p_param, const Variant &p_value) {

	Material *material = material_owner.get(p_material);
	ERR_FAIL_COND(!material);

	if (p_value.get_type() == Variant::NIL) {
		material->params.erase(p_param);
	} else {
		material->params[p_param] = p_value;
	}

	_material_make_dirty(material);
}

Firstly it looks up the text, and secondly it seems to make the material dirty even though it may not have changed.

I'm presuming here with a Sprite3D can it use a sprite atlas, and change the UVs while keeping the material the same?

An alternative might be to do the checks for redundant changes within the renderers / visual server. This might be slightly less efficient but give cleaner code and be a bit more foolproof.

[clayjohn]

I think this is a very good idea. But I don't think it is necessary. _draw() is only called when an update has been queued. Updates are queued when a property changes that would affect the appearance of the Sprite3D.

As you pointed out, the only real issue is for AnimatedSprites because _draw() is called every frame. But the reason _draw() is called every frame is because the texture is changing every frame (or the texture region or something similar). The only real opportunity for optimization is the case where a texture atlas is used and every frame is from the same atlas, then we could potentially save on the rebind. However, the cost is so low, I don't think the added complexity is worth it unless we find out that it is creating a bottleneck.

[tommiemartin]

Amazing work! Really grateful for the effort as this was an unforeseen hiccup in my current project.

You can see the impact here again on a Samsung Galaxy S7 instancing an animated Sprite3d Node. Before fps would significantly drop at only a few nodes where as now we start seeing much smaller dips that begin only when the instances reach about 150.

Godot 3.2.1.stable vs build with PR

[reduz]

I would try to use the Sprite3D materials instead of a custom one, this will reduce state changes a lot when drawing. there is an aPI in VisualServer to obtain them.

[clayjohn]

Updated to use the mesh updating API and skip the overhead in the VisualServer. This updates the mesh arrays directly in the compressed format and should result in even more performance than the first iteration.

I am away from my development computer right now and so I can't provide comparable performance metrics.

[lawnjelly]

Looks good to me 👍 , maybe reduz will have some input on the materials.

[akien-mga]

Changes look good to me, and @reduz approved too, so let's merge!

[akien-mga]

Thanks!

[MagellanicGames]

It's not a bug exactly, but if before in 3.2.2 if you had a texture applied in the Sprite3D and a material override, it still rendered the texture.
In 3.2.3 rc5 you need to assign the texture to the material override's texture albdeo.
Sprite3d.zip

The Sprite 3d on the left has a texture applied in the Sprite3d node with a material assigned in material override.
The one on the left has had to have that texture applied in the material override

Edit: Here is an image of the same example project in 3.2.2:

[clayjohn]

@MagellanicGames thanks! The docs have already been updated to reflect the change. The online docs will be updated when 3.2.3 releases.

For reference, there were two reasons for the change:

1. it makes more sense for material override to actually override the material and
2. it's impossible for meshes to override the texture the way that ImmediateGeometry does.

For now see the docs PR:
#41088

[MagellanicGames]

Awesome, glad to hear and that totally makes sense. It's why I was hesitant about calling it a bug because..well that is indeed how it should work.

[2blackbar]

Cmon dudes, this is still an issue in Godot_v3.3.2-stable_win64
Wheres proper nightly with fix ?

[akien-mga]

Cmon dudes, this is still an issue in Godot_v3.3.2-stable_win64

No it's not, you're commenting on a merged PR which changed Sprite3D to use meshes back in 3.2.3-stable. This change is still included in all 3.3.x releases.

If you experience an issue in 3.3.2, please open a new bug report with the information requested by the issue template.