Blocks toml #351
Blocks toml #351
Conversation
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.
src/data_processing.rs
Outdated
| @@ -141,7 +141,7 @@ pub fn generate_world( | |||
| let total_iterations_grnd: f64 = (scale_factor_x + 1.0) * (scale_factor_z + 1.0); | |||
| let progress_increment_grnd: f64 = 30.0 / total_iterations_grnd; | |||
|
|
|||
| let groundlayer_block = if args.winter { SNOW_BLOCK } else { GRASS_BLOCK }; | |||
| let groundlayer_block = if args.winter { BLOCKS.by_name("snow_block").unwrap() } else { BLOCKS.by_name("grass_block").unwrap() }; | |||
There was a problem hiding this comment.
This is... not the most elegant API. 🤷🏼♂️
|
This is a good idea, but right now the implementation has some drawbacks:
|
benjamin051000
force-pushed
the
blocks-toml
branch
from
8af2e3b to
f121fae
Compare
Meh, I guess the performance penalty is small...
True. That might be worth looking into. It still requires a global variable, though, and it seems clunky and tough to massage your struct into something nice.
Returning an Option is probably the right thing to do. You can always just |
|
Would it be better to use something like If we had |
TODO squash this and reword
benjamin051000
force-pushed
the
blocks-toml
branch
from
f121fae to
ce5bcf4
Compare
|
Trying to get static_toml working is not the best, but I asked about potentially modifying it to generate Option types for optional values here: cptpiepmatz/static-toml#6 |
|
The other thing we can do (I think) is just fill the toml file with null values in every entry. It's a horrible thing to do, but it'll give us a much cleaner generated struct. Then we can have it be a |
|
For the time being, honestly maybe we should just unwrap, and get rid of it later when static_toml supports what we need it to support |
|
Hey man, sorry again for the delay! Just resolved some merge conflicts so we can finally get this merged hah. Will let this run through the CI, I probably didn't catch everything at my first commit haha |
|
I'm really sorry, I'm afraid I need your help here hahha! I already tried a few times locally rebasing your PR to get it up to date with the main branch, but I fail miserably. I can offer you to freeze any integration in the main branch until we get this integrated. Let me know if there are certain files I can help you with to get this done asap :D |
|
You're alive, haha! This PR is not ready for merging. Note that my last commit was "down to 7 errors". What I meant by that comment is that it currently isn't building. I wasn't even sure if you wanted this feature or not, but it seems like you do, so I'll get it synced with main |
|
No worries, take your time. I like the approach of cleaning up the block definitions which is why I'm so tempted to get this running :D We just need to make sure that the performance stays roughly the same and maybe find a solution for the unwrapping stuff. So if you think it's not so critical to get it into the next release, we can take our time for it. I just didn't want to leave you hanging for so long! Thanks a lot for all your effort! :) |
The current
block-defs.rsis not a great solution. Instead, put all block info in a toml file, and pull it in (ideally, at compile time). Then, simply query the one you need either by its ID or name.Note
The things to look at in this PR are
block_definitions.rsandblocks.toml. Everything else is just myfailed(it's getting there!) attempt to update all the API calls, and is now outdated anyway. We will get there eventually.All, let me know if you like this change, and see my below comments for more info.