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.

Sign up for GitHub

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

Jump to bottom

fix a station event weighting bug #33584

Merged
merged 3 commits into from
Dec 6, 2024
Merged

fix a station event weighting bug #33584

merged 3 commits into from
Dec 6, 2024

Conversation

IProduceWidgets
Copy link
Contributor

@IProduceWidgets IProduceWidgets commented Nov 26, 2024 •
edited
Loading

About the PR

Due to how the weighted event selector works, FindEvent(..), only integer weights resulted in the event ever getting selected.

Why / Balance

bugfix

Technical details

image

Media

Requirements

Breaking changes

Changelog

🆑

  • fix: StationEvents with very low weights will now actually appear.

@github-actions github-actions bot added size/XS Denotes a PR that changes 0-9 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. Changes: No C# Changes: Requires no C# knowledge to review or fix this item. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 26, 2024
@IProduceWidgets
Copy link
Contributor Author

We could support floats, but the solution here is kinda elegant, so Id rather just restrict to int in the component so people dont make the mistake.

@slarticodefast
Copy link
Member

Your changelog is formatted wrong.

@IProduceWidgets
Copy link
Contributor Author

Your changelog is formatted wrong.

50% of the time it works every time.

@slarticodefast
Copy link
Member

Why does it cast the weight to an integer anyways?
Weights are already floats.
grafik
And it looks like it FindEvent could be easily changed to use floats.

@IProduceWidgets
Copy link
Contributor Author

Why does it cast the weight to an integer anyways? Weights are already floats. grafik And it looks like it FindEvent could be easily changed to use floats.

I assume its to avoid a random float and dealing with the bounds.
Consider 5 events with weights 1, 1, .2, .8, 1, and 3 random draws of 2, 3, and 4.

Draw a selects the 2nd event
Draw b selects the 4th event
Draw c selects the 5th event

With integer random draws its not possible to get all the events.

@slarticodefast
Copy link
Member

Couldn't the random integer just be changed to a random float from 0 to sumOfWeights?
Or is my math wrong here?

@github-actions github-actions bot added size/S Denotes a PR that changes 10-99 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Nov 26, 2024
@IProduceWidgets
Copy link
Contributor Author

Couldn't the random integer just be changed to a random float from 0 to sumOfWeights? Or is my math wrong here?

No it can, just a stylistic difference I suppose.
I went ahead and kept floats using NextFloat()

@slarticodefast slarticodefast self-assigned this Nov 26, 2024
@slarticodefast slarticodefast added the S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. label Nov 26, 2024
@slarticodefast
Copy link
Member

slarticodefast commented Nov 26, 2024 •
edited
Loading

Good catch with the bug. We would likely have never noticed and it looks like it affects some other events as well, for example the Urist swarm.
Thanks for the fix!

@slarticodefast slarticodefast added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. P1: High Priority: Higher priority than other items, but isn't an emergency. labels Nov 26, 2024
@ScarKy0 ScarKy0 added T: Bugfix Type: Bugs and/or bugfixes D3: Low Difficulty: Some codebase knowledge required. A: Core Tech Area: Underlying core tech for the game and the Github repository. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 26, 2024
@chromiumboy
Copy link
Contributor

The math makes sense, thanks for the fix

@chromiumboy chromiumboy merged commit 3e0b93d into space-wizards:master Dec 6, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slarticodefast slarticodefast slarticodefast approved these changes

Assignees

@slarticodefast slarticodefast

Labels
A: Core Tech Area: Underlying core tech for the game and the Github repository. Changes: No C# Changes: Requires no C# knowledge to review or fix this item. D3: Low Difficulty: Some codebase knowledge required. P1: High Priority: Higher priority than other items, but isn't an emergency. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. size/S Denotes a PR that changes 10-99 lines. T: Bugfix Type: Bugs and/or bugfixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IProduceWidgets @slarticodefast @chromiumboy @ScarKy0