Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
aavenel opened this issue Mar 25, 2020 · 5 comments · May be fixed by #358
Open

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

aavenel opened this issue Mar 25, 2020 · 5 comments · May be fixed by #358
Labels
help wanted Issues identified as good community contribution opportunities

Comments

@aavenel
Copy link

aavenel commented Mar 25, 2020

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
Copy link
Member

jbevain commented Mar 25, 2020

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

originalfoo added a commit to CitiesSkylinesMods/AdditiveShader that referenced this issue Jun 9, 2020
> `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`

Source:  microsoft/Microsoft.Unity.Analyzers#52
@originalfoo
Copy link

originalfoo commented Jun 16, 2020

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 sailro added the help wanted Issues identified as good community contribution opportunities label Apr 26, 2021
@sailro
Copy link
Member

sailro commented Apr 26, 2021

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

@KisaragiEffective
Copy link

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

@sailro
Copy link
Member

sailro commented Oct 7, 2024

Sure, feel free to submit a PR.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues identified as good community contribution opportunities
Projects
None yet
5 participants