microsoft / Microsoft.Unity.Analyzers

Roslyn analyzers for Unity game developers
MIT License
684 stars 72 forks source link

Avoid array copy when querying Length for mesh.vertices #52

Open aavenel opened 4 years ago

aavenel commented 4 years ago

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!

jbevain commented 4 years ago

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

originalfoo commented 4 years ago

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...

sailro commented 3 years ago

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

KisaragiEffective commented 1 month ago

I think I have implemented this. Is the repository still alive? I'll submit a PR if this is.

sailro commented 1 month ago

Sure, feel free to submit a PR.

Thanks!