Microsoft.Unity.Analyzers icon indicating copy to clipboard operation
Microsoft.Unity.Analyzers copied to clipboard

Avoid array copy when querying Length for mesh.vertices

Open aavenel opened this issue 5 years ago • 5 comments

mesh.vertices returns a copy of all vertices. If you have code like mesh.vertices.Length, this will be quite inefficient. It's much better to use mesh.vertexCount or cache mesh.vertices if you plan to use data from vertices array later.

I have seen this at least one time in real code where there was code similar to this:

for(int i=0; i<mesh.vertices.Length; i++)
{
//some stuff
}

There is also a much worse offender which look a bit similar but is perhaps another issue:

    void Update()
    {
        Mesh mesh = GetComponent<MeshFilter>().mesh;

        for (var i = 0; i < mesh.vertexCount; i++)
        {
            //This will copy a new vertices and normals arrays at each iteration
            mesh.vertices[i] += mesh.normals[i] * Mathf.Sin(Time.time);
        }
    }

while correct code should be something like this:

    void Update()
    {
        Mesh mesh = GetComponent<MeshFilter>().mesh;
        Vector3[] vertices = mesh.vertices;
        Vector3[] normals = mesh.normals;

        for (var i = 0; i < vertices.Length; i++)
        {
            vertices[i] += normals[i] * Mathf.Sin(Time.time);
        }

        mesh.vertices = vertices;
    }

Unity Mesh API reference : https://docs.unity3d.com/ScriptReference/Mesh.html

I don't really know how to create an analyzer for this yet. I want to be sure that this is something useful and in the scope of this project first!

aavenel avatar Mar 25 '20 13:03 aavenel

Hi, thank you for submitting this. That's a fantastic idea for a rule.

jbevain avatar Mar 25 '20 20:03 jbevain

I think this should also apply to mesh.colors, which also copies the array before returning it.

While there isn't a separate count property for colors, it's my understanding that the array will (should?) always be same length as the .vertexCount?

If that is the case, using int numColors = mesh.vertexCount would avoid the silent array copy incurred by int numColors = mesh.colors.Length.

If it is not the case, then at least a warning about the array copy incurred by accessing mesh.colors.

Edit: Also mesh.colors32, mesh.tangents, etc...

originalfoo avatar Jun 16 '20 00:06 originalfoo

Automatically adding "help wanted" tag for stale issues identified as good community contribution opportunities

sailro avatar Apr 26 '21 22:04 sailro