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

Slinqify ALL the LINQs #836

Closed
wants to merge 2 commits into from
Closed

Conversation

ts826848
Copy link
Contributor

No description provided.

@jwvanderbeck
Copy link
Contributor

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.

@ts826848
Copy link
Contributor Author

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.

@rsparkyc
Copy link
Member

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/
The poster is smooth_p (https://www.reddit.com/user/smooth_p), and it looks like he's still active. Perhaps you can reach out and he could post his slinq code to github? That way we can at least see what he's doing.

@ts826848
Copy link
Contributor Author

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

@rsparkyc
Copy link
Member

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

@ts826848
Copy link
Contributor Author

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

@rsparkyc
Copy link
Member

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

@ts826848
Copy link
Contributor Author

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.
@pap1723
Copy link
Contributor

pap1723 commented Nov 16, 2018

Ok, what does this do and is it needed to merge?

@ts826848
Copy link
Contributor Author

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

@pap1723
Copy link
Contributor

pap1723 commented Nov 16, 2018

Thanks, I will close this for now.

@pap1723 pap1723 closed this Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants