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

[MP] seed RNG in modules #1125

Merged
merged 2 commits into from
Sep 29, 2023
Merged

[MP] seed RNG in modules #1125

merged 2 commits into from
Sep 29, 2023

Conversation

Razish
Copy link
Member

@Razish Razish commented Sep 2, 2022

This should fix unwanted side-effects introduced by 2bcf85c.
Each module has its own copy of the initial RNG seed (holdrand), but only the engine's copy was being updated on init.
There were many effects of this, notably the "random" spawns on a map_restart were actually consistent, since the game module gets reloaded.

See also: long discussion on JKCommunity discord over a couple days starting here breaking down the original rand usage and explanation of why this is the best fix (as opposed to reverting 2bcf85c).

Copy link

@bartrpe bartrpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me

@Razish Razish force-pushed the rand-module-seed branch from ecb70f6 to d859734 Compare May 11, 2023 12:47
@Razish Razish requested a review from ensiform May 11, 2023 12:54
@Razish Razish requested a review from a team September 22, 2023 10:54
@bartrpe
Copy link

bartrpe commented Sep 27, 2023

how's the status on this one? i think no new developments have been made and this version can be pushed?

Copy link
Member

@SomaZ SomaZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the lengthy discussion we had on discord reagarding this, I think this is fine for merging. This is the closest we can get with random numbers compared to the original executable considering different build systems. Randoms are called all over the place, also with user interactions, so theres no pattern that can be recreated for the random values.

Copy link
Contributor

@Daggolin Daggolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As nobody found any indications of easily recognizable patterns and as the vanilla modules seem to have not used the bg_lib.c version of rand there is probably no need to revert the old commit and this should be enough to counter the mentioned side effects.

To reference the mentioned discussion and given the every compile feels different discussions over the years I can understand why someone would want to revert the mentioned commit, but I suppose if someone is afraid of hidden side effects it's easier to recompile modules from a codebase closer to the vanilla code than to try and undo the cleanup made in OpenJK.

So let's use this PR. If something comes up in the future the two commits could still be reverted.

@bartrpe
Copy link

bartrpe commented Sep 28, 2023

Considering the lengthy discussion we had on discord reagarding this, I think this is fine for merging. This is the closest we can get with random numbers compared to the original executable considering different build systems. Randoms are called all over the place, also with user interactions, so theres no pattern that can be recreated for the random values.

Honestly you made a point that I think is often overlooked here, sometimes it feels like people feel pushing some new thing to repo is a lifetime commitment :P

@Razish Razish merged commit 5f86d1a into master Sep 29, 2023
@Razish Razish deleted the rand-module-seed branch September 29, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants