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
Cache packed color on Sprite #7222
base: master
Are you sure you want to change the base?
Conversation
vertices[C2] = color; | ||
vertices[C3] = color; | ||
vertices[C4] = color; | ||
vertices[C1] = packedColor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already store the packed color in the vertices, though using it (as we used to) is a bit hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a few issues here that could really bite someone if they used certain, not-uncommon colors (like 50% transparent black, especially when animating a change to alpha). I think the underlying issue (one of several possible) may be that Color.toFloatBits() isn't as fast as it could be. I'm not sure how it could be sped up, really; NumberUtils.floatToIntColor(), which is used by Color.abgr8888ToColor(), can be sped up a tiny bit and keep the same results, though. I described how a year ago. The change would remove a float multiplication at no extra cost, as far as I can tell. That said, it takes a LOT of individual float multiplications to make a dent in framerate.
I don't think we can do anything that would make any noticeable change in delta time for the Android app with particles in question. Going from using 5% of CPU time to using 3% might garner a 1-FPS improvement, and that assumes we can nearly-halve the time spent handling packed colors.
Thanks for the review and pointing out the problems of float comparison and corner cases.
Don't take that 5% as a reference, it may be higher or lower in some scenarios and more testing may be needed but, if it was the case that on a "normal" mobile game we can get a 1fps improvement across the board with such as small change it'd be great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! I like that we can use the != on floats fast-path almost all the time.
When profiling my apps I've realized the transformations between Color and packed color float have a pretty high CPU usage (in my case about 5%) as it may occur hundreds of times per frame.
Caching packed color in
Sprite
allows the optmization not to do the conversion when the color is the same and avoid lots of unnecessary conversions in some cases. You can see it only makes this check onsetAlpha()
andsetPackedColor()
, the reason being that adding athis.color.equals(color)
check would cause 2 calls totoIntBits()
making the optimization worthless.With this change we open the possibility for further optimizations on the consumer side. For example
ParticleEmitter
currently callssetColor()
every frame on each single particle. It could now callsetAlpha()
orsetPackedColor()
instead saving resources when theColor
of theParticle
hasn't changed.@NathanSweet It'd be great if you could have a look