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

Fix replace-with-self migration data in mods #50966

Merged
merged 4 commits into from
Aug 23, 2021

Conversation

eltank
Copy link
Contributor

@eltank eltank commented Aug 21, 2021

Summary

Bugfixes "Fix replace-with-self migration data in mods"

Purpose of change

Fixes #50953
The Generic Guns mod had migration entries that replaced some item ids with identical item ids. This caused those items to become blacklisted and removed from recipes.
Aftershock also has one such migration entry. It is unclear what ill effects it causes if any, but it is safe to remove.

Describe the solution

  1. Remove the broken migration entries from Generic Guns
  2. Add JSON validation to prevent this from happening again
  3. Modify generic_guns_validator.py to not complain about 1
  4. Remove broken migration entries from Aftershock
  5. Remove all references to Advanced UPS (clean up after 4)

Describe alternatives you've considered

Testing

(Without any changes) Start a new game with the "Generic Guns" mod enabled, boost all skills to 5, save.

Try to craft "shotgun trap", see that the shotgun component is missing. Search for "shotgun" recipes, find only 1.

image

image

After adding the validation, get this error on game load:

image

After removing the offending JSON, the game loads and the recipe for "shotgun trap" looks correct. You can also craft many more shotgun recipes.

image

Additional context

I haven't tested all the mods, just Generic Guns, Magiclysm and Aftershock. The new JSON check may cause other mods with bad migration data to stop loading, but if so they'll be easy to fix. The CI appears to check all the mods, that's how I found out about Aftershock.

@actual-nh actual-nh added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Mods: Generic Guns Anything to do with Generic Guns labels Aug 21, 2021
@eltank
Copy link
Contributor Author

eltank commented Aug 22, 2021

Hmm, so now I'm getting:

Missing Generic Guns migrations for these types of guns:
slamfire_shotgun
slamfire_shotgun_d

The above errors can be resolved by either adding suitable migrations to Generic Guns or adding to the whitelists of things not requiring migration in tools/json_tools/generic_guns_validator.py

wat

@eltank
Copy link
Contributor Author

eltank commented Aug 22, 2021

Ping @jbytheway who wrote generic_guns_validator.py to take a look.
I'll probably just make changes to that script but I don't fully understand why the script exists or what it's doing.

These are already "generic" so they're being modified rather than
replaced by the GG mod.
@jbytheway
Copy link
Contributor

Yes, I think adding to the whitelist is probably the right call.

@eltank
Copy link
Contributor Author

eltank commented Aug 22, 2021

Looks like the CI is going to check the other mods too. Neat. Now I need to deal with:

`MIGRATION` attempting to replace entity with itself: adv_UPS_off

[
  {
    "id": "adv_UPS_off",
                        ^
    "type": "MIGRATION",
    "replace": "adv_UPS_off",
    "//": "Overrides migration from CDDA."

Looks like AFS wants to keep the advanced UPS which has been obsoleted in main. But doing it this way is not going to work, or rather it's going to cause problems due to the item being blacklisted.

@eltank eltank changed the title Fix generic guns migration data Fix replace-with-self migration data in mods Aug 22, 2021
@eltank
Copy link
Contributor Author

eltank commented Aug 22, 2021

After talking to @John-Candlebury on the discord, it looks like the best thing to do here is to just remove the advanced UPS from Aftershock (it was nerfed and obsoleted by #48608).

@eltank
Copy link
Contributor Author

eltank commented Aug 22, 2021

Started a new AFS game to see what happens to a save file in which the player is wearing an advanced UPS, loaded after this change. The answer is: it's simply replaced with a wearable normal UPS.

@ZhilkinSerg ZhilkinSerg merged commit 1451cea into CleverRaven:master Aug 23, 2021
@eltank eltank deleted the fix_generic_guns_migration branch August 23, 2021 18:26
@Maleclypse
Copy link
Member

Someone sent me this from this early morning “ aftershock has a game crashing bug with the new update - the migration makes the ups_off replace itself i guess? and the game doesnt like that so it kicks you back to main screen” I wasn’t sure if this is fixed by this PR or if this is the problem this PR fixed.

@eltank
Copy link
Contributor Author

eltank commented Aug 25, 2021

Someone sent me this from this early morning “ aftershock has a game crashing bug with the new update - the migration makes the ups_off replace itself i guess? and the game doesnt like that so it kicks you back to main screen” I wasn’t sure if this is fixed by this PR or if this is the problem this PR fixed.

Without a bug report or at least an error message I can't tell what's up with that. This PR removes adv_ups_off from AFS, so anyone who had one would have it replaced by ups_off after updating.

Ah, but it's possible that this PR breaks other mods that are not in our repository because it introduces an additional JSON check that may prevent the game from starting (see 3rd screenshot).

Affected users or mod owners should remove migration entries of the form:

  {
    id: "X",
    type": "MIGRATION",
    "replace": "X"
  }

from the mod json file that the error message points to.

If they're doing this in order to "keep alive" an item that has been obsoleted in main, they need to give that item a new ID in their mod (and search-and-replace old_id with new_id in their mod files).

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` [JSON] Changes (can be) made in JSON Mods: Generic Guns Anything to do with Generic Guns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic Guns mod breaks some recipes
5 participants