-
Notifications
You must be signed in to change notification settings - Fork 165
Block Properties refactor #2063
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
Conversation
Implemented a sorted array system for rarely-used block properties, significantly reducing memory usage. Main changes: - Added generic `SortedBlockProperties<size, DataType>` type with binary search for efficient access - Moved rarely-used properties (`allowOres`, `gui`, `tickEvent`, `touchFunction`, `blockEntity`) from full [65536] arrays to sorted arrays with ~100 entries - Organized all block properties into `BlockProps` structure for better code readability - Added helper functions `isSortedProp()` and `resetSortedProperties()` for automatic handling of sorted properties Memory savings: ~5 properties × (65536 - 100) entries × type size ≈ 320 KB with current configuration, with possibility for further optimization. Access performance: O(log n) instead of O(1), but n won't ever be higher than ~300, so the difference is negligible in practice. also added .vscode and saves to gitignore I want to work on refactoring block entities in a near future and this is the first part of it.
Removed excessive comments according to style guide.
I don't like some of the formatting rules tbh.
| return .gt; | ||
| } | ||
|
|
||
| pub fn getBlockSortedIdx(self: *const Self, blockId: u32) ?usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be public.
| !/assets/backgrounds/default_background.png | ||
| /assets/cubyz/music | ||
| /assets/cubyz/fonts | ||
| /saves/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should no longer be needed?
Also god damn that missing trailing lf
| zig-out/ | ||
| zig-cache/ | ||
| .zig-cache/ | ||
| .vscode/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDE configurations are strictly forbidden from being mentioned anywhere in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Argmaster I am sorry, what?
- Can you link us to where that is written? I just checked CONTRIBUTING.md and I could not find anything related.
- I hope you can see the ridiculousness of that statement. Because you are forbidden to add project configuration to repo, you are also forbidden to make git ignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psznm Exactly my thoughts on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you are forbidden to add project configuration to repo, you are also forbidden to make git ignore it?
It's pretty easy to do this yourself without adding it to gitignore, there is some more info here: #1851
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty easy to do this yourself without adding it to gitignore, there is some more info here: #1851
That is nice. I didn't know that was even possible. It would be useful to copy the comment from zig .gitignore to this .gitignore.
Still, the statement @Argmaster made without any sort of explanation or suggestion really rubbed me the wrong way. I really think just a little bit more effort to help those who don't know goes a long way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, @QuantumDeveloper we should include that in contribution guidelines. That was "knowledge of the elders" and I didn't exactly remember where to look for that discussion.
| /// with ~100-300 entries instead of creating arrays with maxBlockCount (65536) entries. | ||
| /// Magic Value - Increase it if you need to store more block properties. | ||
| /// Consider creating a separate variable with a comment for a specific property if only one needs a larger size. | ||
| pub const maxSortedBlockProperties: usize = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic value is unfortunately unacceptable, as you don't know how many of them will be added by add-ons. I don't think the scale would be so huge that falling to array storage makes sense, but it should at least be resizeable to meet the demand in worst case scenario.
| /// Consider creating a separate variable with a comment for a specific property if only one needs a larger size. | ||
| pub const maxSortedBlockProperties: usize = 100; | ||
|
|
||
| pub var sortedAllowOres: SortedBlockProperties(maxSortedBlockProperties, bool) = .{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is overqualified, it duplicates the information present in the type.
|
|
||
| pub var sortedAllowOres: SortedBlockProperties(maxSortedBlockProperties, bool) = .{}; | ||
| // TODO: Tick event is accessed like a milion times for no reason. FIX IT | ||
| pub var sortedTickEvent: SortedBlockProperties(maxSortedBlockProperties, TickEvent) = .{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is overqualified, it duplicates the information present in the type.
| pub var sortedAllowOres: SortedBlockProperties(maxSortedBlockProperties, bool) = .{}; | ||
| // TODO: Tick event is accessed like a milion times for no reason. FIX IT | ||
| pub var sortedTickEvent: SortedBlockProperties(maxSortedBlockProperties, TickEvent) = .{}; | ||
| pub var sortedTouchFunction: SortedBlockProperties(maxSortedBlockProperties, *const TouchFunction) = .{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is overqualified, it duplicates the information present in the type.
| pub var sortedBlockEntity: SortedBlockProperties(maxSortedBlockProperties, *BlockEntityType) = .{}; | ||
|
|
||
| /// GUI that is opened on click. | ||
| pub var sortedGui: SortedBlockProperties(maxSortedBlockProperties, []u8) = .{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is overqualified, it duplicates the information present in the type.
| } | ||
| } | ||
|
|
||
| const BlockProps = struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of that struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code organization. Grouping all block properties in one struct makes the code more readable than having them floating in the global namespace. Also, I'll probably make this struct public in later changes when I'm splitting most of the blocks.zig and block_entity.zig code into different files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not C and those variables are not in the global namespace, but rather in the module scope of the module dedicated for this data. Following this relocation logic you should move all the methods in this file into that struct, as they are one big family and then the code would be equivalent to a file struct in a file struct. Files are from Zig perspective equivalent to structs.
Therefore past structure of this file should be interpreted as a struct with only static fields and static methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a lot of complexity for saving 320 kiB (compare too the many gigabytes used by the rest of the game) while also likely degrading performance.
It would have to save at least 100× more memory to be even close to being reasonable.
For #1853 why not just use a single pointer per block type (ideally put into the BlockEntity struct) that points to the block entity data?
|
@IntegratedQuantum For #1853, I don't want to use the monolithic 'single pointer per block type' structure like we currently have for block entities. Instead, I want a more modular approach where each block can have unique functionality composed from smaller pieces. My idea is to define block functionality in .zon files without hardcoding the block itself: Example: chest/_defaults.zig.zonIn this example, .inventory would automatically register all the functions it needs into BlockProps, but by itself it wouldn't allow opening the inventory. For that we'd use the .onInteract property. This way we can easily create new functional blocks by reusing existing code and combining different properties. |
|
You cannot completely remove There are some properties we will need in the future, eg. related to explosions but as of now there are only few we know about. Mods may require more, but your solution doesn't provide modding support for adding new props (past one didn't do that either) Modding is different than add-ons - modding adds code, add-ons add assets. |
|
I want to create a system where you can define new parameters in code and register them with the block_manager at startup, *( compile time #1528 ) so modding will be possible. |
|
A way to save a bit of memory that was briefly considered was using ListUnmanaged, but the problem is that we are using Arena allocator, so that arena will aggregate unused memory on realloc, resulting in 1.5 memory usage compared to necessary capacity. |
Check out rotation modes for how to do this kind of thing at compile-time. |
Do we really want to compile mods with the game ? |
|
Yes, see #1528 |
Ok, works for me, but overall this isn't really the subject of this review, so maybe we can talk about this when I start working on what I've proposed ;D ? I know it has to support modding, so don't worry - I have that in mind. |
|
This would aggregate a lot of logic in one place and solves none of the problems with block entities we have now. |
Summary
Implemented a sorted array system for rarely-used block properties, significantly reducing memory usage.
This is the first step toward addressing #1853.
Main changes
SortedBlockProperties<size, DataType>type with binary search for efficient accessallowOres,gui,tickEvent,touchFunction,blockEntity) from full [65536] arrays to sorted arrays with ~100-300 entriesBlockPropsstructure for better code readabilityisSortedProp()andresetSortedProperties()for automatic handling of sorted propertiesPerformance impact
Memory savings: ~5 properties × (65536 - 100) entries × type size ≈ 320 KB with current configuration, with potential for further optimization.
Access performance: O(log n) instead of O(1), but with n ≤ 300, the difference is negligible in practice.
Future work
Next steps for #1853:
Also added .vscode and saves to .gitignore