Unity-Technologies / ProjectAuditor

Project Auditor is an experimental static analysis tool for Unity Projects.
Other
789 stars 65 forks source link

[ADD] Solid texture analyser #177

Closed emeline-berenguier closed 1 year ago

emeline-berenguier commented 1 year ago

Description

Added an issue when when a solid color texture has a size bigger than 1x1. Fix it by turning it by resize it to 1x1.

Changes made

Add descriptor Add module Add tasks coverage

Notes

Please write down any additional notes, remove the section if not applicable.

Checklist

Before review:

mtrive commented 1 year ago

Color32ToInt seems like quite a lot of code to do a relatively simple conversion. Is it more performant than the alternatives, or intended as a utility method for new analyzers in the future? If so, all good. If not, something like this seems more concise and easier to follow.

var c = pixels[0]; var colorValue = (uint)((c.r<<24)|(c.g<<16)|(c.b<<8)|c.a);

If you decide to keep Color3ToInt, consider making it Color32ToUint - having the top bit as a sign value doesn't really make sense.

I agree the conversion is simple. I would be really curious to benchmark both methods. Perhaps @mtschoen-unity did that (he's the original author of that code). Let's see what he says but for the time being, let's keep it as is.

mtschoen-unity commented 1 year ago

I agree the conversion is simple. I would be really curious to benchmark both methods. Perhaps @mtschoen-unity did that (he's the original author of that code). Let's see what he says but for the time being, let's keep it as is.

I didn't benchmark against that particular conversion. What I had prior to Color32ToInt is 4 == comparisons (one for each color channel) in the inner loop, which was indeed much slower than this approach. Without testing, I'm pretty sure that the struct-based conversion is faster. All it does is a single mem copy, whereas the suggested code does 3 bit shifts and ors, on top of accessing the individual fields. In fact, Color32 has a similar private rgba field which does the same conversion. I'm pretty sure converting converting colors to ints like this is "a thing." :)

As for int vs uint, I don't think it makes a difference. We're not doing math on these values (in fact I point out in the readme that you can't do multiplication) so all we care about is checking whether the values are equal. AFAICT, we could use float and get the same result except for a few edge cases.