GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fixed config seed by ELginas · Pull Request #516 · feather-rs/feather

Conversation

@ELginas

@ELginas ELginas commented Jan 16, 2022

Copy link
Copy Markdown

TITLE - Replace

Status

  • Ready
  • Development
  • Hold

Description

Removed hardcoded seed in main.rs and replaced it with seed from config file.

Related issues

Leave empty if none

Checklist

  • Ran cargo fmt, cargo clippy --all-targets, cargo build --release and cargo test and fixed any generated errors!
  • Removed unnecessary commented out code
  • Used specific traces (if you trace actions please specify the cause i.e. the player)

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Defman
Defman reviewed Jan 16, 2022
View reviewed changes
Comment thread 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();

@Defman Defman Jan 16, 2022

Copy link
Copy Markdown
Member

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.

Should use the following hash algorithm s[0]*31^(n-1) + s[1]*31^(n-2) + … + s[n-1] which is javas String.hashCode().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Iaiao Iaiao Jan 16, 2022

Copy link
Copy Markdown
Contributor

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.

Here's my implementation from #487: https://github.com/Iaiao/feather/blob/210fed57a4f54681c35d9d49956b1ced8f78c8fb/feather/server/src/main.rs#L95-L101

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ELginas ELginas Jan 16, 2022

Copy link
Copy Markdown
Author

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Defman Defman Jan 16, 2022

Copy link
Copy Markdown
Member

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.

Maybe add a few tests, to make sure that the code behave as javas String.hashCode().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Iaiao Iaiao Jan 16, 2022

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ELginas
ELginas force-pushed the main branch from 6341e91 to 5acbc4d Compare January 16, 2022 15:15
Defman
Defman reviewed Jan 16, 2022
View reviewed changes
Comment thread feather/worldgen/src/lib.rs
}
}

pub fn seed_from_string(s: &str) -> u64 {

@Defman Defman Jan 16, 2022

Copy link
Copy Markdown
Member

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 maybe be an i64, such that negative seeds also work as expected?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ELginas ELginas Jan 16, 2022

Copy link
Copy Markdown
Author

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.

Everywhere in feather seeds are defined by u64

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Defman Defman Jan 16, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Yes, but it should be parsed as i64 and then cast to u64. I thin that's how vanilla Minecraft works.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Iaiao Iaiao Jan 16, 2022

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Defman Defman Jan 18, 2022

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ELginas
Fixed config seed
4e0e7a4
@ELginas
ELginas force-pushed the main branch from 5acbc4d to 4e0e7a4 Compare January 16, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Defman Defman Defman left review comments
+1 more reviewer
@Iaiao Iaiao Iaiao left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@ELginas @Defman @Iaiao
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.