Street signs, namespaced blocks #570
Conversation
…files Fix merge conflict markers in rust sources
Fix clippy warnings
|
retrigger-benchmark |
|
⏱️ Benchmark run finished in 36m 16s 📈 Compared against baseline: 135s 🚨 This PR drastically worsens generation time. You can retrigger the benchmark by commenting |
|
Thanks for the feedback - I'll work on improving the patch. |
…without-changing-functionality Improve performance of highway generation and chunk serialization
|
retrigger-benchmark |
Fix clippy lint warnings
Use is_multiple_of for clippy compliance
|
retrigger-benchmark |
|
Don't worry too much, there might be a bug in the benchmarking bot currently. Btw thanks for the contribution and the AI disclaimer, I'll review it soon! |
| use std::path::PathBuf; | ||
| use std::sync::atomic::{AtomicU64, Ordering}; | ||
|
|
||
| const DATA_VERSION: i32 = 3700; |
There was a problem hiding this comment.
Do you know where the constant comes from? Is this constant bound to a specific version of Minecraft?
There was a problem hiding this comment.
Yes, it's the Java Edition data value: https://minecraft.fandom.com/wiki/Java_Edition_data_values/Protocol_and_data_versions
3700 = Java Edition 1.20.4
| /// Construct a `Block` from an arbitrary namespaced string. | ||
| /// | ||
| /// The string is stored in a global cache to obtain a `'static` lifetime. | ||
| pub fn from_str(name: &str) -> Block { |
There was a problem hiding this comment.
Intentionally leaks memory with Box::leak() to create 'static references. No mechanism to clean up these leaked strings during program lifetime. Could accumulate significant memory usage with dynamic block creation?
There was a problem hiding this comment.
I tried setting the AI coder (codex web) to get rid of this, but after 3 tries with timeouts (it gave up), I asked for the pros and cons of removing the Box::leak(). What follows is the response:
Switching Block to store a Cow<'static, str> would eliminate the need to leak owned strings in Block::from_str and could make it easier to accept dynamically generated block identifiers without synchronizing through the global cache. However, the rest of the crate treats Block as an inexpensive, trivially copyable value. That assumption is baked into large swaths of the codebase: arrays of blocks are copied out by indexing, helper tables are repeated with [BLOCK; N] initializers, and world-editing data structures iterate over [Block; 4096] palettes by value.
Pros
Removes the lifetime and leaking workaround in Block::from_str, letting owned names stay owned and potentially collapsing the mutex-protected cache entirely.
Gives future features (e.g., importing user-defined blocks) a natural place to store owned identifiers instead of forcing everything through 'static literals.
Cons
Block losing Copy would ripple through all array-based helpers, requiring clone()/cloned() at each random-selection site (WINDOW_VARIATIONS, get_random_floor_block, agricultural crop picks, etc.).
Core world-editing structures rely on copy semantics for performance: initializing [AIR; 4096], iterating with for (i, &block), and hashing (Block, Option) tuples all assume cheap copies; rewriting them to clone would add both code churn and per-block overhead.
Every cache lookup like *block in Block::from_str or DashMap keys would need explicit cloning, touching concurrency-critical paths and risking regressions without clear functional gain.
Recommendation
Unless there is a pressing requirement for many distinct, owned block identifiers, the maintenance and performance cost of de-Copying Block seems to outweigh the benefit. The existing cache already bounds the leak to at most one allocation per distinct name, so the pragmatic risk is modest compared to the extensive refactoring this change would force across generation and world-editing code.
| let all = [element.clone()]; | ||
| generate_highways(&mut editor, &element, &args, &all); | ||
|
|
||
| for x in [200, 400, 600, 800, 1000] { |
There was a problem hiding this comment.
Test assumes signs are placed at specific coordinates without accounting for the actual road geometry. Test may be brittle and fail if road placement algorithm changes slightly
|
I already addressed three of my comments with commits, however there are some things we need to clarify before merging to avoid introducing regressions :) |
2 participants
Refactor Block to use string names; add road sign placement
Disclosure: used AI to build this patch