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

Enable automatic testing of a few more mods: #36433

Merged
merged 23 commits into from
Dec 28, 2019
Merged

Conversation

BevapDin
Copy link
Contributor

SUMMARY: None

Fixes a few JSON problems (mostly unvisited members) and enables testing for mods that work now.


There are many errors in the JSON files that are simply ignored because our JSON handling is so forgiving:

A "looks_like": [ "still" ] is ignored, because the game expects a string. This however does not even generate an error, is just ignores the data.

Similar: some code uses has_bool( "foo" ) or even simpler has_member( "foo" ) to determine the behaviour. It completely ignores the data (even the type) of the member. So "foo": false and "foo": true and "foo": { "maybe": "never" } are all the same to the game.

This PR changes that:

  • JSON object members are no longer considered visited when only the existence / their type has been checked. That means calling has_member or has_bool is not enough, one has to actually call get_bool to mark the member as visited.

  • Many checks for a JSON object member now ignore the type. They just check via has_member instead of via has_<type>. When the member exists, the code usually tries to read it anyway, and that reading will trigger an error if the member is not of the expected type. Previously JSON like "calories": "54" was ignored (test for has_int( "calories" ) was false). Now it triggers an error ("tried to read integer, but found string"), which informs the JSON author about their mistake.

  • Convert some silent debug messages (only printed into the debug log file, but never shown on the console) into loud messages: printed on the console and requiring user input to skip them. This informs the user that their mods use outdated syntax and encourages them / the mod authors to convert it to the new syntax to get rid of the debug message.

@ZhilkinSerg ZhilkinSerg added <Bugfix> This is a fix for a bug (or closes open issue) [JSON] Changes (can be) made in JSON Code: Build Issues regarding different builds and build environments Code: Tests Measurement, self-control, statistics, balancing. Mods Issues related to mods or modding [C++] Changes (can be) made in C++. Previously named `Code` labels Dec 25, 2019
{ "x": -4, "y": 0, "chance": 10, "items": { "item": "dog_food", "container-item": "can_medium" } },
{ "x": -4, "y": 1, "chance": 10, "items": { "item": "cat_food", "container-item": "can_food" } },
{ "x": -4, "y": 1, "chance": 10, "items": { "item": "cat_food", "container-item": "can_food" } },
{ "x": -4, "y": 0, "chance": 10, "//": { "item": "dog_food", "container-item": "can_medium" } },
Copy link
Contributor

Choose a reason for hiding this comment

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

Why comment instead of items here and in three lines below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because spawning items via an elaborated syntax similar to item group is not implemented for vehicles. This system allows simple arrays of strings (item type ids), or a just a single string, but nothing else.

Alternatively one has to create an explicit item group that allows container-items.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. I thought it was a mapgen.

data/json/flags.json Outdated Show resolved Hide resolved
@AMurkin
Copy link
Contributor

AMurkin commented Dec 25, 2019

Invalid indentation here: 157400c.

"info" and "default" are optional and ignored anyway.
This informs the user that their mods use outdated syntax and encourage them / the mod authors to convert them to the new syntax to get rid of the debug message.
…ence.

This ensures the member type is checked and reported if it does not fit the expected type.

It also means `"foo": false` is properly recognized as such (instead of being interpreted the same as `"foo": true`).
Now it only checks whether a member of the given name exists. The type is ignored hereby. The type is checked later, when the value of the member is read. If the type is not what we expect, an error will be generated.
The message will be shown anyway when the member is actually accessed.
The JSON object member "om_special" is never actually loaded, so it should not be required.
"items" must be a string or an array of strings.
The "size" entry is no longer used and it is now ignored.
As required by the style we use.
@@ -77,7 +77,7 @@
"destination": "dairy_farm_isherwood_W",
"start": {
"effect": [ { "u_add_var": "u_have_barry_escape", "type": "general", "context": "meeting", "value": "yes" } ],
"assign_mission_target": { "om_terrain": "dairy_farm_isherwood_W", "om_special": "Isherwood Farms", "reveal_radius": 3 }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this should still be here, and is retrieved in https://github.com/CleverRaven/Cataclysm-DDA/pull/36433/files#diff-0e0da92a1ece3afd4c1e42acd3a3dc51L375.

As for all the ones struck from the update_mapgen entries: I think the fact that the update_mapgen doesn't use it is an oversight in its implementation rather than useless data on the entries, as without it, it can't differentiate between specials that contain the same overmap terrain (which granted is relatively rare) and thus will target any instance of the omt, but I guess we'll restore the data whenever the implementation gets updated.

@ZhilkinSerg
Copy link
Contributor

diff --git a/data/json/npcs/isherwood_farm/NPC_Carlos_Isherwood.json b/data/json/npcs/isherwood_farm/NPC_Carlos_Isherwood.json
index c9427af5c6..2057b0d585 100644
--- a/data/json/npcs/isherwood_farm/NPC_Carlos_Isherwood.json
+++ b/data/json/npcs/isherwood_farm/NPC_Carlos_Isherwood.json
@@ -195,12 +195,7 @@
     "end": {
       "opinion": { "trust": 1, "value": 1 },
       "effect": [ { "u_buy_item": "leather_armor_horse", "count": 1 } ],
-      "update_mapgen": [
-        {
-          "om_terrain": "horse_farm_isherwood_4",
-          "set": [ { "point": "furniture", "id": "f_anvil", "x": 6, "y": 12 } ]
-        }
-      ]
+      "update_mapgen": [ { "om_terrain": "horse_farm_isherwood_4", "set": [ { "point": "furniture", "id": "f_anvil", "x": 6, "y": 12 } ] } ]
     }
   },
   {
diff --git a/data/json/npcs/isherwood_farm/NPC_Chris_Isherwood.json b/data/json/npcs/isherwood_farm/NPC_Chris_Isherwood.json
index 57d907bf2f..5ac7a4e57e 100644
--- a/data/json/npcs/isherwood_farm/NPC_Chris_Isherwood.json
+++ b/data/json/npcs/isherwood_farm/NPC_Chris_Isherwood.json
@@ -186,12 +186,7 @@
     },
     "end": {
       "opinion": { "trust": 5, "value": 5 },
-      "update_mapgen": [
-        {
-          "om_terrain": "cabin_isherwood",
-          "place_nested": [ { "chunks": [ "cabin_isherwood_update" ], "x": 3, "y": 1 } ]
-        }
-      ]
+      "update_mapgen": [ { "om_terrain": "cabin_isherwood", "place_nested": [ { "chunks": [ "cabin_isherwood_update" ], "x": 3, "y": 1 } ] } ]
     }
   }
 ]
diff --git a/data/json/npcs/refugee_center/surface_visitors/NPC_old_guard_representative.json b/data/json/npcs/refugee_center/surface_visitors/NPC_old_guard_representative.json
index 0eaf670f9c..67d19d4b8f 100644
--- a/data/json/npcs/refugee_center/surface_visitors/NPC_old_guard_representative.json
+++ b/data/json/npcs/refugee_center/surface_visitors/NPC_old_guard_representative.json
@@ -131,10 +131,7 @@
     "value": 50000,
     "start": {
       "update_mapgen": [
-        {
-          "om_terrain": "evac_center_19",
-          "place_npcs": [ { "class": "evac_guard3", "x": 12, "y": 12, "target": true } ]
-        },
+        { "om_terrain": "evac_center_19", "place_npcs": [ { "class": "evac_guard3", "x": 12, "y": 12, "target": true } ] },
         {
           "om_terrain": "evac_center_7",
           "place_npcs": [ { "class": "scavenger_hunter", "x": [ 9, 15 ], "y": [ 9, 15 ] } ]
diff --git a/data/json/npcs/tacoma_ranch/NPC_ranch_foreman.json b/data/json/npcs/tacoma_ranch/NPC_ranch_foreman.json
index 752c45aa6c..0ced7bbef4 100644
--- a/data/json/npcs/tacoma_ranch/NPC_ranch_foreman.json
+++ b/data/json/npcs/tacoma_ranch/NPC_ranch_foreman.json
@@ -251,10 +251,7 @@
           "place_furniture": [ { "furn": "f_bookcase", "x": 17, "y": 11 } ],
           "place_npcs": [ { "class": "ranch_farmer_1", "x": 19, "y": 20 } ]
         },
-        {
-          "om_terrain": "ranch_camp_66",
-          "place_npcs": [ { "class": "ranch_woodcutter_1", "x": 4, "y": 11 } ]
-        }
+        { "om_terrain": "ranch_camp_66", "place_npcs": [ { "class": "ranch_woodcutter_1", "x": 4, "y": 11 } ] }
       ]
     }
   },
@@ -586,10 +583,7 @@
     },
     "end": {
       "update_mapgen": [
-        {
-          "om_terrain": "ranch_camp_59",
-          "place_nested": [ { "chunks": [ "tacoma_commune_clinic_10" ], "x": 0, "y": 2 } ]
-        },
+        { "om_terrain": "ranch_camp_59", "place_nested": [ { "chunks": [ "tacoma_commune_clinic_10" ], "x": 0, "y": 2 } ] },
         {
           "om_terrain": "ranch_camp_60",
           "place_nested": [ { "chunks": [ "tacoma_commune_chopshop_10" ], "x": 0, "y": 4 } ]
@@ -621,10 +615,7 @@
     },
     "end": {
       "update_mapgen": [
-        {
-          "om_terrain": "ranch_camp_59",
-          "place_nested": [ { "chunks": [ "tacoma_commune_clinic_11" ], "x": 0, "y": 2 } ]
-        },
+        { "om_terrain": "ranch_camp_59", "place_nested": [ { "chunks": [ "tacoma_commune_clinic_11" ], "x": 0, "y": 2 } ] },
         {
           "om_terrain": "ranch_camp_60",
           "place_nested": [ { "chunks": [ "tacoma_commune_chopshop_11" ], "x": 0, "y": 4 } ]
@@ -674,10 +665,7 @@
     },
     "end": {
       "update_mapgen": [
-        {
-          "om_terrain": "ranch_camp_49",
-          "place_nested": [ { "chunks": [ "tacoma_commune_junk_shop_12" ], "x": 0, "y": 9 } ]
-        },
+        { "om_terrain": "ranch_camp_49", "place_nested": [ { "chunks": [ "tacoma_commune_junk_shop_12" ], "x": 0, "y": 9 } ] },
         {
           "om_terrain": "ranch_camp_60",
           "place_nested": [ { "chunks": [ "tacoma_commune_chopshop_12_done" ], "x": 0, "y": 4 } ]
@@ -714,14 +702,8 @@
     },
     "end": {
       "update_mapgen": [
-        {
-          "om_terrain": "ranch_camp_49",
-          "place_nested": [ { "chunks": [ "tacoma_commune_junk_shop_13" ], "x": 0, "y": 9 } ]
-        },
-        {
-          "om_terrain": "ranch_camp_66",
-          "place_npcs": [ { "class": "ranch_barber", "x": 5, "y": 3 } ]
-        },
+        { "om_terrain": "ranch_camp_49", "place_nested": [ { "chunks": [ "tacoma_commune_junk_shop_13" ], "x": 0, "y": 9 } ] },
+        { "om_terrain": "ranch_camp_66", "place_npcs": [ { "class": "ranch_barber", "x": 5, "y": 3 } ] },
         {
           "om_terrain": "ranch_camp_70",
           "translate_ter": [ { "from": "t_underbrush", "to": "t_dirt", "x": 0, "y": 0 } ],
@@ -793,10 +775,7 @@
       "failure": "It was a lost cause anyways..."
     },
     "end": {
-      "update_mapgen": {
-        "om_terrain": "ranch_camp_51",
-        "place_nested": [ { "chunks": [ "tacoma_commune_bar_15" ], "x": 0, "y": 0 } ]
-      }
+      "update_mapgen": { "om_terrain": "ranch_camp_51", "place_nested": [ { "chunks": [ "tacoma_commune_bar_15" ], "x": 0, "y": 0 } ] }
     }
   },
   {
@@ -823,10 +802,7 @@
     },
     "end": {
       "update_mapgen": [
-        {
-          "om_terrain": "ranch_camp_51",
-          "place_nested": [ { "chunks": [ "tacoma_commune_bar_16_done" ], "x": 0, "y": 0 } ]
-        },
+        { "om_terrain": "ranch_camp_51", "place_nested": [ { "chunks": [ "tacoma_commune_bar_16_done" ], "x": 0, "y": 0 } ] },
         {
           "om_terrain": "ranch_camp_52",
           "place_nested": [ { "chunks": [ "tacoma_commune_greenhouse_16" ], "x": 2, "y": 10 } ]
diff --git a/data/mods/FictonalWeapons/fic_mods.json b/data/mods/FictonalWeapons/fic_mods.json
index d47ab0891c..5c33122176 100644
--- a/data/mods/FictonalWeapons/fic_mods.json
+++ b/data/mods/FictonalWeapons/fic_mods.json
@@ -32,13 +32,7 @@
     "color": "light_gray",
     "location": "underbarrel",
     "mod_targets": [ "pistol", "smg" ],
-    "gun_data": {
-      "ammo": "flammable",
-      "skill": "launcher",
-      "dispersion": 320,
-      "durability": 10,
-      "clip_size": 300
-    },
+    "gun_data": { "ammo": "flammable", "skill": "launcher", "dispersion": 320, "durability": 10, "clip_size": 300 },
     "flags": [ "FIRE_50" ],
     "dispersion_modifier": 25,
     "min_skills": [ [ "weapon", 2 ], [ "launcher", 2 ] ]

src/color_loader.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments Code: Tests Measurement, self-control, statistics, balancing. [JSON] Changes (can be) made in JSON Mods Issues related to mods or modding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants