Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd mutation/trait groups, trait blacklisting, and NPC mutation templates #22487
Conversation
SpencerMichaels
added some commits
Nov 24, 2017
SpencerMichaels
changed the title
Add mutation/trait groups and trait blacklisting
Add mutation/trait groups, trait blacklisting, and NPC mutation templates
Nov 25, 2017
BevapDin
reviewed
Nov 25, 2017
| #include "string.h" | ||
| #include "string_id.h" | ||
|
|
||
| typedef std::string Trait_group_tag; |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
This should be a string_id of some kind. Item groups use raw std::string because they have not yet been changed to use string_id, but string_id is preferable as provides a common interface and more type safety.
BevapDin
reviewed
Nov 25, 2017
| #include "string_id.h" | ||
|
|
||
| typedef std::string Trait_group_tag; | ||
| typedef std::vector<trait_id> Trait_list; |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
Should probably be inside the namespace, as it is mostly used inside of that namespace.
BevapDin
reviewed
Nov 25, 2017
| /** | ||
| * Check whether the given group ID has been defined. | ||
| */ | ||
| bool group_is_defined( const Trait_group_tag &gid ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
This function, for example, can be provided via the string_id interface. Instead of group_is_defined(somegroup), one can write somegroup.is_valid().
BevapDin
reviewed
Nov 25, 2017
| typedef std::vector<Trait_group_tag> RecursionList; | ||
|
|
||
| Trait_creation_data( int _probability ) : probability( _probability ) {} | ||
| virtual ~Trait_creation_data() {}; |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
You can write = default; instead of the empty function body. It's sometimes more efficient. And there are some semantic differences, generally default is not worse than a default, but potentially better.
BevapDin
reviewed
Nov 25, 2017
|
|
||
| trait_id id; | ||
|
|
||
| virtual Trait_list create( RecursionList &rec ) const; |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
override is missing here (some compilers can warn you about this). And you don't need to add virtual to a function that overrides (it is redundant). But our style guide does not force any style.
BevapDin
reviewed
Nov 25, 2017
| // Probability, used by the parent object. | ||
| int probability; | ||
| private: | ||
| // not implemented |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
Than just delete them (add = delete;) instead of making them private. The code in "item_group.h" is old, so it doesn't use features from C++11, new code like this should use them where appropriate.
BevapDin
reviewed
Nov 25, 2017
| Trait_list result; | ||
| int p = rng( 0, sum_prob - 1 ); | ||
| for( const auto &creator : creators ) { | ||
| p -= ( creator )->probability; |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
•
Contributor
The brackets are unnecessary. Also applies at many other places.
This comment has been minimized.
This comment has been minimized.
douglasdennis
Nov 25, 2017
This is a bit of stylistic feedback isn't it? Is this something covered in a style guide regarding brackets for single statement blocks?
This comment has been minimized.
This comment has been minimized.
kevingranade
Nov 25, 2017
Member
I think he means the parentheses around creator, ( creator )->probability;, brackets are mandatory.
This comment has been minimized.
This comment has been minimized.
SpencerMichaels
Nov 26, 2017
Author
Contributor
This is the style that I saw frequently in item_factory.cpp and item_group.cpp, I thought it was kind of weird too. I'll fix those in the new PR I split off from this.
BevapDin
reviewed
Nov 25, 2017
| } | ||
|
|
||
| std::vector<Trait_group_tag> mutation_branch::get_all_group_names() { | ||
| std::vector<std::string> rval; |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
Gotcha. Why are you returning a vector<string> while the function declaration says it's returning a vector<Trait_group_tag>? This would not happen with string_id.
BevapDin
reviewed
Nov 25, 2017
| TraitGroupMap trait_groups = { | ||
| // An empty dummy group, it will not generate any traits. However, it makes that trait group | ||
| // id valid, so it can be used all over the place without need to explicitly check for it. | ||
| {"EMPTY_GROUP", new Trait_group_collection(100)} |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
Ugh. Raw owning pointers? Please don't do this. If you absolutely need pointers, wrap them in a smart pointer class (for example unique_ptr). Do you actually need a pointer here? Note that the values in a std::map have stable addresses, inserting or removing entries does not affect the address of the remaining values.
BevapDin
reviewed
Nov 25, 2017
| JsonArray jarr = jsobj.get_array( "traits" ); | ||
| while (jarr.has_more()) { | ||
| trait_id id(jarr.next_string()); | ||
| trait_blacklist.insert(id); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
I'm nitpicking now. This could be done in one line, without the id variable: insert( trait_id( next_string() ) ).
BevapDin
reviewed
Nov 25, 2017
| /** | ||
| * Check if a trait with the given ID exists. | ||
| */ | ||
| static bool has_trait( const trait_id &tid ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
reviewed
Nov 25, 2017
| } | ||
|
|
||
| Trait_creation_data* mutation_branch::get_group( const Trait_group_tag &gid ) { | ||
| if (trait_groups.count(gid) > 0) { |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
This is inefficient, the map is searched twice, once via count and a second time via operator[]. Typically this functionality is implemented by calling find(key) and comparing the result to end(), see for example Item_factory::find_template.
BevapDin
reviewed
Nov 25, 2017
| my_traits.clear(); | ||
|
|
||
| // Add fixed traits | ||
| Trait_list tmplist = trait_group::traits_from( type->traits ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
You don't nee this variable, you can iterate over the result of the function cal directly for( ... : traits_from() ).
BevapDin
reviewed
Nov 25, 2017
|
|
||
| JsonArray jarr = stream.get_array(); | ||
| if( default_subtype != "collection" && default_subtype != "distribution" ) { | ||
| debugmsg( "invalid subtype for trait group: %s", default_subtype.c_str() ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
This should probably be an error as well. We can't really continue with an invalid type, we don't know what is meant.
BevapDin
reviewed
Nov 25, 2017
| class Trait_group : public Trait_creation_data | ||
| { | ||
| public: | ||
| typedef std::vector<Trait_creation_data *> CreatorList; |
This comment has been minimized.
This comment has been minimized.
BevapDin
reviewed
Nov 25, 2017
| @@ -556,6 +556,10 @@ trait_id Character::trait_by_invlet( const long ch ) const | |||
|
|
|||
| bool player::mutation_ok( const trait_id &mutation, bool force_good, bool force_bad ) const | |||
| { | |||
| if (mutation_branch::trait_is_blacklisted(mutation)) { | |||
| // This mutation has been blacklisted. | |||
This comment has been minimized.
This comment has been minimized.
BevapDin
Nov 25, 2017
Contributor
Well, isn't that what the condition above already says? What else should the return value true of mutation_branch::trait_is_blacklisted mean? Why add a comment for this obvious behaviour?
kevingranade
reviewed
Nov 25, 2017
| @@ -0,0 +1,148 @@ | |||
| [ | |||
| { | |||
| "//": "This group picks out everal negative and positive traits, with up to 4 rolls of decreasing likelihood.", | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
This all looks like features we want, the only concern is if NPCs don't know how to behave with certain mutations, but since it's opt-in with a mod that's not a major issue. Top level comment, this PR is too big, it needs to be broken up so we can properly review the pieces, but posting the whole shebang can be good for getting an overview so no harm done. I'd suggest:
|
SpencerMichaels
added some commits
Nov 25, 2017
This comment has been minimized.
This comment has been minimized.
|
@BevapDin Thanks for the review! I hadn't realized the @kevingranade Sure, that makes sense. I will leave this PR open for now in case people want to review it further, but I'll submit separate PRs for the different features in a moment. I think it should be fairly trivial to split the |
This was referenced Nov 25, 2017
This comment has been minimized.
This comment has been minimized.
SpencerMichaels
closed this
Nov 26, 2017
kevingranade
reviewed
Nov 26, 2017
| /* Mutation rounds can be specified as either a single distribution: | ||
| * "mutation_rounds" : { "constant" : 1 } | ||
| * or a map of categories to distributions: | ||
| * "mutation_rounds" : [ |
This comment has been minimized.
This comment has been minimized.
kevingranade
Nov 26, 2017
•
Member
This example isn't quite right, it looks more like:
"mutagen_rounds": [
[ "MUTCAT_ANY", { "constant": 1 } ],
[ "MUTCAT_INSECT", { "rng": [1, 3] }
]
SpencerMichaels commentedNov 25, 2017
•
edited
This PR expands the trait/mutation specification for characters to allow complex random trait generation like what can already be done with item groups, while retaining backwards compatibility with the old system. It also lets modders create NPC classes that spawn with predetermined semi-random mutations. I originally designed this to aid in creating better NPC classes for some mutant-related NPC quests I'm working on adding, and I think this could be a big help to modders working on NPCs.
I've been writing C++ and playing Cataclysm for quite some time, but this is my first major contribution to CDDA. I've done my best to follow the existing code/comment style, but I am not too familiar with the codebase as a whole yet, so let me know if there's anything I should change or refactor.
Highlights
Trait groups
Previously, when specifying an NPC class's traits, one could only do so through a format like this:
This is not very flexible, however. Consider, for instance, a case where a modder wants to make an NPC that has Genetically Unstable 2/3rds of the time, and Genetic Chaos 1/3 of the time — but never both at once. With the old system, this is not possible. With this new system, you can do this:
What if you want to give the same set of mutations to multiple NPCs, or choose between multiple sets of mutations at random? You can define a
trait_group— whose syntax is quite similar to anitem_group— and nest arbitrarily complicated invocations of collections and distributions. Collections will choose whether or not to add a trait based on its probability considered in insolation. Distributions will chose one of their traits based on the weights of all the traits summed together. For instance, the example group below is a collection containing a trait and a distribution, with the distribution containing two traits. It will always generate traitOUTDOORSMAN, and half of the time will generate eitherGOODCARDIO(2/3rds chance) orGOODHEARING(1/3rd chance).Trait blacklisting
In addition to the above, I also added blacklisting capability, like so:
If an entry like this is loaded, (a) the character creation menu will not show Bad Knees (b) players and NPCs cannot mutate Bad Knees and (b) Bad Knees will be removed from any trait groups that contain it.
Mutation rounds
I added a new tag,
mutation_rounds, to NPC class templates. This allows you to cause the generated NPC to have mutated along one or more mutation categories an arbitrary number of times. This means that if, say, you want an Insect mutant NPC, you don't have to use the above functionality to manually write out probabilities for each trait, you can make use of the existing mutation system to make him undergo several rounds ofMUTCAT_INSECTmutations before spawn.You can specify mutation rounds in two ways; both use the RNG distribution objects like, for instance, the
bonus_strfield. The first is a shortcut for general mutations (like what you'd get from drinking regular mutagen).The second lets you specify any number of categories. In this example, our NPC always undergoes 1
ANYmutation, as well as 2d3INSECTmutations before spawn.Taken together with the trait groups, you can use the groups to generate NPCs with certain fixed traits and then mutate them to push them further along that semirandom mutation path.
Full changes list
trait_groupJSON object type that is formatted much like anitem_group, allowing modders to group traits together under an identifier, as well as to usecollections anddistributions to perform choices like "one of a set" and "either-or" between mutations.TRAIT_BLACKLISTJSON object type that allows blacklisting traits/mutations. Any blacklisted trait will disappear from the character creation menu and never result from either mutations or trait group instantiation.Trait_Groups_TestandMutant_NPCsmods that I used to test JSON object processing and NPC spawns. If preferred, I can remove them before the PR is merged, but I will leave them for now in case anyone wants to test.Test trait groupentry to the debug menu that allows the user to generate 100 sample rounds of a trait group and see how many of each trait come out of it (again, quite likeTest item group).mutation_roundsNPC class tag that allows modders to spawn pre-mutated NPCs without using the trait group system manually. This allows organic progress down arbitrary mutation paths.Two complete sets of example JSON files, covering all the added functionality, can be found in
/data/mods/Trait_Groups_Testand/data/mods/Mutant_NPCs.Screenshots and Examples
This is the result of a simple roll of 100% genetically unstable and 50% genetic chaos

This is the result of the

outdoorsman_distributiontrait group example above.This is the group automatically generated by the

traitsentry inNC_APIS(seejson/npcs/classes.json:394), the bee mutant NPC class.This is an NPC mutated at spawn via the

MUTCAT_INSECTcategory example above. (He also spawned with some starting traits, which I added under thetraitstag.)This is an NPC mutated at spawn with 10 rounds of

MUTCAT_RAPTOR.