Fixed config seed #516
Fixed config seed #516
Conversation
feather/server/src/main.rs
Outdated
|
|
||
| let seed = 42; // FIXME: load from the level file | ||
| let seed = config.world.seed.parse().unwrap_or_else(|_| { | ||
| let mut hasher = DefaultHasher::new(); |
There was a problem hiding this comment.
Should use the following hash algorithm s[0]*31^(n-1) + s[1]*31^(n-2) + … + s[n-1] which is javas String.hashCode().
There was a problem hiding this comment.
Here's my implementation from #487: https://github.com/Iaiao/feather/blob/210fed57a4f54681c35d9d49956b1ced8f78c8fb/feather/server/src/main.rs#L95-L101
There was a problem hiding this comment.
Here's my implementation from #487: https://github.com/Iaiao/feather/blob/210fed57a4f54681c35d9d49956b1ced8f78c8fb/feather/server/src/main.rs#L95-L101
Wouldn't it be useless for me to continue if you have already implemented this feature in your own fork?
There was a problem hiding this comment.
Maybe add a few tests, to make sure that the code behave as javas String.hashCode().
There was a problem hiding this comment.
Here's my implementation from #487: https://github.com/Iaiao/feather/blob/210fed57a4f54681c35d9d49956b1ced8f78c8fb/feather/server/src/main.rs#L95-L101
Wouldn't it be useless for me to continue if you have already implemented this feature in your own fork?
My implementation doesn't have any tests and won't be merged soon (because it requires all vanilla commands to be implemented), so I think it wouldn't be useless to implement it in a separate pr.
| } | ||
| } | ||
|
|
||
| pub fn seed_from_string(s: &str) -> u64 { |
There was a problem hiding this comment.
This should maybe be an i64, such that negative seeds also work as expected?
There was a problem hiding this comment.
Everywhere in feather seeds are defined by u64
There was a problem hiding this comment.
Yes, but it should be parsed as i64 and then cast to u64. I thin that's how vanilla Minecraft works.
There was a problem hiding this comment.
I think it should be i64 everywhere and only be casted when passing it to RNGs, it would be more convenient for feather or plugin developers who expect -104019040 seed to be negative, and for end users that expect /seed (or seed value in logs / crash reports) to be the same as in config.toml.
There was a problem hiding this comment.
I agree with the above comment, @ELginas do you want to make this change or should I merge this? We can then create a new issue to change the seed type from u64 to i64.
3 participants
TITLE - Replace
Status
Description
Removed hardcoded seed in main.rs and replaced it with seed from config file.
Related issues
Leave empty if none
Checklist
cargo fmt,cargo clippy --all-targets,cargo build --releaseandcargo testand fixed any generated errors!Note: if you locally don't get any errors, but GitHub Actions fails (especially at
clippy) you might want to check your rust toolchain version. You can then feel free to fix these warnings/errors in your PR.