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

36 occurrences where different JSON objects share same type and id #42984

Closed
int-ua opened this issue Aug 17, 2020 · 8 comments · Fixed by #42989
Closed

36 occurrences where different JSON objects share same type and id #42984

int-ua opened this issue Aug 17, 2020 · 8 comments · Fixed by #42989
Labels
[JSON] Changes (can be) made in JSON
Milestone

Comments

@int-ua
Copy link
Contributor

int-ua commented Aug 17, 2020

Describe the bug

While preparing to make tools/json_tools/util.py work with copy-from I've discovered not only that there are >500 instances where same id is used on different JSON objects (which is not ideal but seems to be expected) but also that there are 36 cases where they share both type and id, which is questionable.

Lists by status

Included in the PR:

type id
AMMO oxygen
event_statistic num_broken_bone
harvest demihuman
json_flag ELECTRIC_IMMUNE
ammo_effect LIGHTNING
trait_group Exp_Eng_Electrical_Group
MONSTER mon_wasp_small
effect_type magnesium
MIGRATION l_enforcer_45
ammunition_type oxygen
TOOL tool_anfo_charge
TOOL tool_anfo_charge_act
GENERIC cable_instrument
item_group light_reading
item_group my_precious
item_group mussto_windinst
item_group mussto_stringinst
item_group ic_merch_vending
item_group ic_merch
mission_definition MISSION_ISHERWOOD_JESSE_2
talk_topic TALK_JESSE_CARLOS
talk_topic CWH_EVACUEE_5_IDEAS1
talk_topic TALK_LEAVE_NOW
talk_topic TALK_TEST_SPEAKER_EFFECT_COMPOUND_SENTINEL_CONDITIONAL

@mlangsdorf is fixingfixed these:

type id
vehicle_part seat
vehicle_part seat_leather
vehicle_part reclining_seat
vehicle_part reclining_seat_leather

Intentional:

type id
talk_topic TALK_FRIEND_CONVERSATION

Steps To Reproduce

diff --git a/tools/json_tools/util.py b/tools/json_tools/util.py
index de33d82..61364b5 100755
--- a/tools/json_tools/util.py
+++ b/tools/json_tools/util.py
@@ -3,7 +3,7 @@
 """

 import argparse
-from collections import Counter, OrderedDict
+from collections import Counter, OrderedDict, defaultdict
 from fnmatch import fnmatch
 import json
 import re
@@ -42,6 +42,15 @@ def import_data(json_dir=JSON_DIR, json_fmatch=JSON_FNMATCH):
                             errors.append("Problem parsing data from file %s, reason: expected a list." % json_file)
                     else:
                         data += candidates
+    all_ids = defaultdict(set)
+    for obj in data:
+        obj_id = obj.get('id')
+        obj_type = obj.get('type')
+        if obj_id and not isinstance(obj_id, list):
+            if obj_id not in all_ids[obj_type]:
+                all_ids[obj_type].add(obj_id)
+            else:
+                print(obj_type, obj_id)
     return (data, errors)

Expected behavior

At least type and id together provide enough info to identify a JSON object globally. But then, there is abstract now too. :(

Versions and configuration

My fork, about 40 commits behind right now.

Additional context

#42958
#42959
https://discordapp.com/channels/598523535169945603/598535827169083403/744539312674308138

@jbytheway
Copy link
Contributor

Did you include mod data in this?

@int-ua
Copy link
Contributor Author

int-ua commented Aug 17, 2020

No, should I?

@jbytheway
Copy link
Contributor

I think that some mods deliberately reuse ids to overwrite the base game version of an item (not totally sure if this is a thing or not) so if you had included mods then that might explain some of this, but given that you haven't all these are probably genuine bugs. Thanks for finding them. Someone with permissions should make this issue a blocker for the release, otherwise it's liable to get lost.

@int-ua
Copy link
Contributor Author

int-ua commented Aug 17, 2020

Can someone help with reviewing talk_topic duplicates and deciding what should be done?

@mlangsdorf
Copy link
Contributor

The TALK_FRIEND_CONVERSATION stuff is legal and intentional.

The TALK_LEAVE_NOW repeats look like a bug.

I'll fix the duplicate seat entries.

@int-ua
Copy link
Contributor Author

int-ua commented Aug 17, 2020

I've updated OP and split the table by status.

int-ua referenced this issue Aug 17, 2020
Allow specifying mapgen_update as a dialogue effect.  Each mapgen_update
effect has a location description that uses the same format as
missiondef's assign_mission_target, and 1 or more mapgen_update_ids that
will be performed on that overmap position.
@int-ua
Copy link
Contributor Author

int-ua commented Aug 17, 2020

On AMMO oxygen: original was added here and then a duplicate was added here. I'm not sure if I should remove the original, please advise. @KorGgenT

@int-ua
Copy link
Contributor Author

int-ua commented Aug 17, 2020

Removed the old one in the PR.

@anothersimulacrum anothersimulacrum added the [JSON] Changes (can be) made in JSON label Aug 17, 2020
@ZhilkinSerg ZhilkinSerg added this to the 0.F milestone Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants