-
Notifications
You must be signed in to change notification settings - Fork 217
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
Slinqify ALL the LINQs #836
Conversation
Are you aware that Smooth is very old and defunct? Unless something resurfaced lately, I would be hesitant to rely on it in newer Unity versions. |
Yeah, I noticed that Smooth seems to have vanished from the internet. I was somewhat unsure about using it, since it seems few people on #RO have experience with it, and there wasn't much mention on the Unity forums, but being able to use the LINQ style without the GC penalty just seemed too good to pass up. Figured that (a.k.a. hoping that) the C# side of things is more likely to be backwards-compatible, too, since the source code (minus libraries) isn't specific to Unity. Not sure if it has any bugs though. In any case, I'll defer to you guys on whether using Slinq is a good idea. If not, I'll just turn these into regular loops and submit another PR. |
So, looks like this guy posted about it here on reddit about 3 years ago: https://www.reddit.com/r/gamedev/comments/24sx1x/smoothfoundations_mit_licensed_unity_package_incl/ |
@rsparkyc As far as I can tell the source code is already on github at pdo400/smooth.foundations. PR #1 seems to provide the strongest hint that this is the right repo. |
Ahh, perhaps. I had assumed that the author's name would be Oren Novotny, since that's what's in this post: https://archive.codeplex.com/?p=slinq |
@rsparkyc Huh, haven't seen that page before. Looks like it's something pretty different, at least; the zip I got from the download button doesn't seem to contain any code whatsoever, oddly enough. |
Yeah I noticed (and was disappointed) by that. I'd have to look at the source you pointed at (which i'm pretty sure is the right source) and see how it works, but my guess is it's pretty non-unity specific (at least in the ways you're using it). Finding time for such things lately is another story entirely... I miss RO... |
Quick grep shows that the only mentions of Unity in non-test code are for logging, so hopefully you're right. Come back! We miss you <3 |
Hopefully keeps the LINQ style while avoiding garbage. Some of the functions take two-arg lambdas and an additional argument to avoid having to allocate to capture an external variable. This results in some pretty bad variable names, unfortunately. Where(x => <cond>).FirstOrDefault() can be replaced with FirstOrDefault(x => <cond>) for simple lambdas. Unfortunately, Slinq doesn't provide a FirstOrDefault() implementation that takes a two-arg lambda and an additional argument, so Where(x => <cond>).FirstOrDefault() is used to avoid allocations. In one case, a LINQ statement was replaced with a function call, since the LINQ statement was exactly the same as that inside the called function. Ended up replacing a FirstOrDefault() with First() since the code accessed a member variable without checking for null first, so an empty result would result in an exception either way. First() makes it a bit clearer that something is expected, though. Also clean up using statements a bit.
Hopefully a bit clearer this way, and only checks TechIsEnabled once instead of once per item in the loop. Guess the JIT should be capable of hoisting the TechIsEnabled check out of the loop, but this way feels more readable.
Ok, what does this do and is it needed to merge? |
@pap1723 It replaces LINQ stuff with equivalent stuff from the Slinq library, which is supposed to provide more garbage-friendly versions of LINQ methods. It's not necessary to merge, and from the above conversation it's not clear that using Slinq is a good idea anyways. |
Thanks, I will close this for now. |
No description provided.