devilutionX icon indicating copy to clipboard operation
devilutionX copied to clipboard

Add alternate RNG and proposed API for versioning Random Number Generators

Open ephphatha opened this issue 4 years ago • 3 comments

This builds on the work done by @qndel in #2084. I have not updated any of the call sites identified in that PR as I wanted to make it obvious that this does not change existing code.

This is more of a discussion point at the moment. I was thinking that namespaces would be a good way to delineate behaviour. I've implemented "vanilla" to cover the base game logic and bugs. "randomV1" is based on #2084 and uses STL concepts to add convenience functions as discussed in #2193 and #2206.

The API design is intended to be common between versions. The next step would be to update the vanilla namespace functions to align member functions of vanilla::RandomEngine. The reason for having this class is to allow creating a RandomEngine object and passing it in to the level/item generation code (for example) to make it clearer how a given object/dungeon layout is generated.

Looking at code such as quests.cpp:InitQuests() makes me think a "vanillaSafe" namespace which fixes the negative number bug would be useful, but this would be a future enhancement iff this proposal is acceptable.

ephphatha avatar Jun 28 '21 12:06 ephphatha

The main changes in this PR are the addition of RandomEngine as a class in engine/random.hpp (and implementation of global functions in random.cpp), versioning namespaces, and updated test cases showing usages. I have updated all uses of RNG functions to refer to the vanilla:: namespaced versions because I don't like having unbuildable commits. I can split that if desired.

@qndel would I be able to add you as a co-author for this PR? I didn't want to imply you had signed off on this but I do want to acknowledge your work in this space.

ephphatha avatar Jun 28 '21 12:06 ephphatha

Added an example of how this could be used to make it easier to ensure code which relies on pseudo-random sequences is functioning correctly. This PR is already massive so I didn't want to make it worse by actually using the new API, but hopefully this gives context for why I think this change is worthwhile.

ephphatha avatar Jun 29 '21 13:06 ephphatha

The coverage result is pretty amazing for this PR :) image (All changed code is tested, and total increased by 0.12%)

AJenbo avatar Jul 02 '21 13:07 AJenbo