chrisdill / raylib-cs

C# bindings for raylib, a simple and easy-to-use library to learn videogames programming
http://www.raylib.com/
zlib License
824 stars 73 forks source link

Bunnymark refactored and cleaned up. #249

Closed thygrrr closed 4 months ago

thygrrr commented 4 months ago

This PR is a small update to Bunnymark:

Functional changes:

image

Structural & stylistic changes:

Feedback wanted:

To keep cognitive complexity below my (arbitrary) own code style guidelines, I used an Enumerable.Range LINQ expression to spawn the Bunnies, alternatively we could:

foreach (ref var bunny in bunnies.Slice(bunnyCount, BunnyIncrement))
{
    bunny = new();
}

... but it felt slightly wasteful due to the prescribed bracing style, and I use the range syntax [ .. ] elsewhere, so the Slice doubly sticks out (but the range syntax would make this line very long). Therefore:

Enumerable.Range(0, BunnyIncrement)
      .Select(_ => new Bunny())
      .ToArray()
      .CopyTo(bunnies[bunniesCount..]);

... appeared to read better for me, and is also considered good style in many projects.

An alternative to this would be to JUST set the bunny positions here, and use the Range in the Initialization to fill the entire Bunny array. This would require re-thinking the "Remove bunnies" behaviour slightly, so cognitive complexity wouldn't improve further (but readability might).

chrisdill commented 4 months ago

@thygrrr Thanks for the PR! Example feels nicer to use. As for spawning them, using range is okay though I prefer it avoid additional allocations from creating the array etc.

thygrrr commented 4 months ago

@thygrrr Thanks for the PR! Example feels nicer to use. As for spawning them, using range is okay though I prefer it avoid additional allocations from creating the array etc.

Indeed this creates allocations, though these are Gen 0 and therefore practically free.

(it's a bit of a superstition harkening from older .NET / Boehm GC days that all temporary allocations are the devil - nowadays, objects that stay alive are what makes the GC and the Marshal work hard because then they need to move memory around to compact the heap)

Either way, I decided to rewrite it with the foreach approach, because I am old and prefer my runtime allocation free, too. 🧓

I settled on the range syntax (not the Slice method):

foreach (ref var bunny in bunnies[bunniesCount..(bunniesCount+BunnyIncrement)])
{
    bunny = new();
}
bunniesCount += BunnyIncrement;

I ran this through Rider DPA and I can see only the initial alloc for the bunnies Array/Span now. 😺 And the string interpolations from the debug display, which are negligible (and also Gen 0)

image

Now I just need to admit to myself that "14" is too low a complexity threshold and this warning will go away, too.
(this is an editorconfig on my side only, and doesn't affect the build) image

chrisdill commented 4 months ago

Thanks, merged!