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

cleanup game of life #1443

Merged
merged 15 commits into from
Jan 14, 2021
Merged

cleanup game of life #1443

merged 15 commits into from
Jan 14, 2021

Conversation

JAicewizard
Copy link
Contributor

This cleans some things up like using ? where applicable and removing unnecessary code.

@JAicewizard
Copy link
Contributor Author

I need to get the vscode rust extension to work again. It just doesnt do anything useful.

@@ -57,7 +57,6 @@ impl Grid {
// death by loneliness or overcrowding
if life && (n_lives_around < 2 || n_lives_around > 3) {
indices_to_mutate.push(pos);
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst it is true that the code will perform identically without this continue, keeping it in might communicate intent better. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I actually thought it did the opposite, having a break/continue within a loop makes it harder to reason about.
After looking at the idiomatic implementation (https://rustwasm.github.io/book/game-of-life/implementing.html) I think using a match might actually be easiest to reason about?

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the wait! A few little questions but this looks good!

};
use std::sync::Arc;

const GRID_SIZE: usize = 40;
const GRID_SIZE: usize = 41;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so that the background grid is not aligned. If it is aligned you get straight lines as cels which is a bit boring, and also doesn't highlight that it is actually a grid. This makes it so that neighboring cells (almost?? I dont know for sure anymore) always have a different colour.

Comment on lines 32 to 36
const C0: Color = Color::from_rgba32_u32(0xEBF1F7); //Color::rgb(235, 241, 247)
const C1: Color = Color::from_rgba32_u32(0xA3FCF7); //Color::rgb(162,252,247)
const C2: Color = Color::from_rgba32_u32(0xA2E3D8); //Color::rgb(162,227,216)
const C3: Color = Color::from_rgba32_u32(0xF2E6F1); //Color::rgb(242,230,241)
const C4: Color = Color::from_rgba32_u32(0xE0AFAF); //Color::rgb(224,175,175)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that using Color::rgb would be better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, but sadly that isnt possible. I made them already but they're not const so decided to put them as comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right I meant Color::rgb8. That's the const one that takes u8s:

const C0: Color = Color::rgb8(0xEB, 0xF1, 0xF7); 
// same as:
const C0: Color = Color::from_rgba32_u32(0xEBF1F7);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also actually fix up these names while we're in here, like BG could be BACKGROUND and then I'd try and give more meaningful names to the other colors too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made it an array, much simpler.

@cmyr cmyr added the S-waiting-on-author waits for changes from the submitter label Dec 13, 2020
@JAicewizard
Copy link
Contributor Author

I thought I left replied for this ages ago!!! now they're all gone...

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@cmyr cmyr merged commit ffef21d into linebender:master Jan 14, 2021
@xStrom xStrom removed the S-waiting-on-author waits for changes from the submitter label Jan 18, 2023
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.

4 participants