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

Add API for restoring Pseudorandom and PcgRandom state #14123

Merged
merged 3 commits into from Jan 16, 2024

Conversation

sfence
Copy link
Contributor

@sfence sfence commented Dec 18, 2023

Add compact, short information about your PR for easier understanding:

To do

This PR is Ready for Review.

How to test

Run /unittests command from devtest game and minetest --run-unittests command from system.

@wsor4035 wsor4035 added @ Script API Feature ✨ PRs that add or enhance a feature labels Dec 18, 2023
@Zughy Zughy added the Concept approved Approved by a core dev: PRs welcomed! label Dec 20, 2023
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Looks good apart from this. (Small note: I don't really care what happens to PseudoRandom as it is strictly inferior to PcgRandom, see #14180).

src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
games/devtest/mods/unittests/random.lua Outdated Show resolved Hide resolved
src/noise.h Outdated Show resolved Hide resolved
src/noise.h Outdated Show resolved Hide resolved
src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
src/unittest/test_random.cpp Outdated Show resolved Hide resolved
@sfence
Copy link
Contributor Author

sfence commented Dec 29, 2023

@appgurueu @sfan5 Should be resolved now.

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Otherwise the code looks fine. I'm not happy with how PseudoRandom uses int rather than a fixed-size type (which might be related to bugs like #14237), but that is out of scope for this PR to fix.

src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Tested, works. Thanks for the PR!

Include review and formatting feedback.
@sfence
Copy link
Contributor Author

sfence commented Jan 15, 2024

Rebased, conflicts solved.

Failure for clang_14 looks to be related to check failure:

 (server) [PASS] test_node_metadata - 0ms
(server) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
(server) Unit Test Results: PASSED
(server)     0 / 35 failed tests.
(server)     Testing took 12450ms total.
(server) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Waiting for done timed out
Error: Process completed with exit code 1.

src/noise.h Outdated Show resolved Hide resolved
games/devtest/mods/unittests/misc.lua Outdated Show resolved Hide resolved
src/script/lua_api/l_noise.cpp Outdated Show resolved Hide resolved
src/unittest/test_random.cpp Outdated Show resolved Hide resolved
src/unittest/test_random.cpp Outdated Show resolved Hide resolved
src/unittest/test_random.cpp Outdated Show resolved Hide resolved
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Looks good other than that

src/noise.cpp Outdated Show resolved Hide resolved
src/unittest/test_random.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 added >= Two approvals ✅ ✅ Action / change needed Code still needs changes (PR) / more information requested (Issues) and removed One approval ✅ ◻️ labels Jan 15, 2024
@sfan5 sfan5 removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 15, 2024
@appgurueu appgurueu merged commit ceaa7e2 into minetest:master Jan 16, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Script API >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lua api: PseudoRandom: peek at current rng state to allow save/restore
5 participants