Skip to content
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

Fix seed command output #403

Merged
merged 1 commit into from
Dec 24, 2024
Merged

Conversation

kralverde
Copy link
Contributor

Description

Prints the seed as a i64 instead of a u64

Testing

Ran /seed with a negative seed

Checklist

Things need to be done before this Pull Request can be merged.

  • Code is well-formatted and adheres to project style guidelines: cargo fmt
  • Code does not produce any clippy warnings: cargo clippy
  • All unit tests pass: cargo test

@Snowiiii
Copy link
Member

@kralverde
Copy link
Contributor Author

I'm pretty sure that https://github.com/Snowiiii/Pumpkin/blob/cc0f30848ea597ef702558cb1e5fc57ed2a20372/pumpkin-world/src/world_gen/seed.rs#L4 should be also an i64 then

No, internally our seed is a u64, it doesn't make sense for it to be signed. Converting from u64 to i64 is a no-op anyway because it has the same bits underneath

@Snowiiii
Copy link
Member

I'm pretty sure that https://github.com/Snowiiii/Pumpkin/blob/cc0f30848ea597ef702558cb1e5fc57ed2a20372/pumpkin-world/src/world_gen/seed.rs#L4
should be also an i64 then

No, internally our seed is a u64, it doesn't make sense for it to be signed. Converting from u64 to i64 is a no-op anyway because it has the same bits underneath

But why it should be unsigned ?. We may also want to use negative seeds internally, Or save them

@kralverde
Copy link
Contributor Author

I'm pretty sure that https://github.com/Snowiiii/Pumpkin/blob/cc0f30848ea597ef702558cb1e5fc57ed2a20372/pumpkin-world/src/world_gen/seed.rs#L4

should be also an i64 then

No, internally our seed is a u64, it doesn't make sense for it to be signed. Converting from u64 to i64 is a no-op anyway because it has the same bits underneath

But why it should be unsigned ?. We may also want to use negative seeds internally, Or save them

IMO It doesn't make sense to me conceptually for it to be signed. There's not concept of positive or negative for what is effectively a bit string for our RNG. I think having it signed was a limitation of Java.

We can always add a serialize method to the seed struct for writing and stuff to do to no-op as i64 to minimize confusion.

@Snowiiii
Copy link
Member

Mhh Okay.
I also wonder if

pub seed: String,
could may be a i64 directly so we don't have to parse it from a String

@kralverde
Copy link
Contributor Author

Mhh Okay.

I also wonder if https://github.com/Snowiiii/Pumpkin/blob/9b184f73b99c6b7b97cf4ee0fe08be84d497b33a/pumpkin-config/src/lib.rs#L70 could may be a i64 directly so we don't have to parse it from a String

Users can input strings as seeds tho

@Snowiiii
Copy link
Member

Snowiiii commented Dec 22, 2024

Mhh Okay.
I also wonder if https://github.com/Snowiiii/Pumpkin/blob/9b184f73b99c6b7b97cf4ee0fe08be84d497b33a/pumpkin-config/src/lib.rs#L70
could may be a i64 directly so we don't have to parse it from a String

Users can input strings as seeds tho

I'm not sure what you mean, a seed is always a number isn't it ?, If you mean to just using " ", this isn't really an argument for me, I think its nicer to just force the user to put a raw number instead of using a String and then parsing is back into a number

@kralverde
Copy link
Contributor Author

kralverde commented Dec 22, 2024

Mhh Okay.

I also wonder if https://github.com/Snowiiii/Pumpkin/blob/9b184f73b99c6b7b97cf4ee0fe08be84d497b33a/pumpkin-config/src/lib.rs#L70

could may be a i64 directly so we don't have to parse it from a String

Users can input strings as seeds tho

I'm not sure what you mean, a seed is always a number isn't it ?, If you mean to just using " ", this isn't really an argument for me, I think its nicer to just force the user to put a raw number instead of using a String and then parsing is back into a number

But like, you can have a string be the seed and it gets hashed into a numerical value. So you are right in that they are ultimately numbers, but if you go into Minecraft you can type a sentence into the set seed and it will work. Java basically goes:

  • empty string -> random
  • number -> parse to i64
  • other -> hash string

@Snowiiii
Copy link
Member

Overall looks good.
Thanks @kralverde

@Snowiiii Snowiiii merged commit 5e60dba into Pumpkin-MC:master Dec 24, 2024
9 checks passed
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