Skip to content

Conversation

@benjamin051000
Copy link
Contributor

@benjamin051000 benjamin051000 commented Jan 26, 2025

I'm not sure why these were initally functions. I think they are better off as global arrays, since the function would initialize a new vec every time its called, whereas this only initializes it once.

TODO

  • Test: I tested it with one small section of a city and it appears to work. I wouldn't by any means call this thoroughly-tested.

QUARTZ_BRICKS,
]
}
pub static BUILDING_CORNER_VARIATIONS: [Block; 20] = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be worth considering making these const instead of static, but I'm not sure. In the future, someone can see if it makes a performance difference.

@benjamin051000
Copy link
Contributor Author

This is mostly overshadowed by #351. We can keep it open or merge it, but when #351 is ready for review that one will modify what's going on here. lol

@louis-e
Copy link
Owner

louis-e commented Mar 10, 2025

This is mostly overshadowed by #351. We can keep it open or merge it, but when #351 is ready for review that one will modify what's going on here. lol

Both approaches look good to me and seem to be a better implementation compared to the current state. Would you prefer #351 over this one? There isn't any time pressure from my side since the next release still needs a few more feature merges, so we could work on finishing #351. Having a better solution than the constant required unwrap calls would be great, like you already mentioned. Really cool work and documentation from your side btw 😎

@benjamin051000
Copy link
Contributor Author

#351 is definitely the preferred thing to do, so merge this or close it, I don't have a preference

I'm not sure why these were initally functions. I think they are better
off as global arrays, since the function would initialize a new vec
every time its called, whereas this only initializes it once.
@benjamin051000
Copy link
Contributor Author

@louis-e Once CI passes, let's merge this. I don't know when/if I'll finish #351 so may as well merge this work I suppose

@louis-e louis-e merged commit e1869fb into louis-e:main Apr 12, 2025
3 checks passed
@benjamin051000 benjamin051000 deleted the refactor-block-defs branch April 12, 2025 22:51
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.

2 participants