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

[MAGICLYSM] Consolidate mutation folders and fix missing dragon mutations #52464

Closed
wants to merge 5 commits into from

Conversation

Terrorforge
Copy link
Contributor

@Terrorforge Terrorforge commented Oct 26, 2021

Summary

Infrastructure "Fixed some issues with certain mutations not being included in the Black Dragon category, consolidated the Magiclysm mutation files into one folder"

Purpose of change

Currently, the Magiclysm mutation files are split between two folders ("traits" and "mutations"), and the two mutation categories (Manatouched and Black Dragon) are split off into their own separate files using copy-and-extend to add existing mutations to their categories. This causes problems for the dragon line, as it attempts to extend Greater Mana Efficiency, Greater Mana Regeneration and Mana Vortex to add them to its own category. Because these are defined in the same mod, this causes some kind of overwrite issue, which results in those mutations not being included in the category.

Additionally, the fact that there are two folders is very confusing, and the naming conventions of some of the files don't help. Notably, the file containing the black dragon mutations is called "mutations.json", whereas the general file containing the mana-related mutations is called "mutation.json", without an 's'. This is presumably the reason a single stray Chimera mutation ended up in the black dragon file. When I notified the code owner @KorGgenT of this state of affairs, they considered it unacceptably messy, and I concur.

Describe the solution

  1. Removed the "copy-from" + "extend" of the mana mutations from the black dragon files.
  2. Added the necessary category and threshreq tags directly to the relevant mutations, including the Mana Vortex prerequisites, which previously had a threshreq only for the Manatouched threshold
  3. Moved the "Uncanny Dodge" mutation from the dragon file to the generic file
  4. Put all the files from the "traits" folder into the "mutations" folder
  5. Renamed several files to make it clear what each one is, e.g. renaming "mutations" to "dragon_mutations" and "mutation" (no 's') to "mutations" to match the vanilla name for "file that contains miscellaneous mutations".

Describe alternatives you've considered

When I raised this issue on the dev discord, I wanted to move the files, but I was told to simply implement the fixes to the dragon mutations and leave the file structure as it is, since moving and renaming files will cause problems with tracking their change history. This is a valid solution that would fix the actual player-facing bug without opening us up to a number of other annoying issues - but the current setup is really ugly, and very confusing. It's already caused one mutation to get misplaced, and it's hardly going to get better. I personally think it's bad enough that I'd rather rip that band-aid off right now, but more importantly, so does KorGgenT, and on something like this I'm always going to defer to their judgement as code owner.

Testing

  1. Start Magiclysm game.
  2. Spawn in syringe and a pile of black dragon serum.
  3. Inject self with serum until you stop gaining new mutations.
  4. Note that you have the Greater Mana Efficiency, Greater Mana Regeneration and Mana Vortex, which you will not if you do the same thing on the current release.
  5. Also note that moving and renaming the files has not caused anything to explode.
  6. Check the magiclysm/mutations and confirm that you more or less understand what the new file names mean.

NOTE: There seems to be an unrelated bug that can cause a crash when you try to inject a serum that's in the pocket of an item of clothing that's destroyed by gaining a mutation, so if you're gonna perform this test I recommend wielding the syringe and taking all your clothes off before you start injecting.

Additional context

While I was troubleshooting, I noticed that simply placing all the files in the same folder fixed the problem with the dragon mutations. It then turned out that whether the copy-extend worked correctly or not seemed to depend on which file came first in an alphabetical list. Specifically, if the file with the extends was first alphabetically, the mutations wouldn't work.

I'm told that the JSON-loading code isn't supposed to care about which order files are, which suggests there might be something wrong with it. But that is beyond my current ability, and in any case far outside the scope of this PR.

Moving the relevant dragon mutations to other files to avoid duplicates.
Combining the "mutations" and "traits" folders
Moving a stray Chimera mutation from the dragon file to the general file
The "mutations" file changes look weird because originally "mutations" was the name of the file with the dragon mutations and "mutation" (no 's') was the name of the file with assorted mutations. I've renamed them this way for consistency with the vanilla naming conventions.
@Terrorforge Terrorforge requested a review from KorGgenT as a code owner October 26, 2021 15:04
@BrettDong BrettDong added Mods: Magiclysm Anything to do with the Magiclysm mod [JSON] Changes (can be) made in JSON labels Oct 26, 2021
@KorGgenT
Copy link
Member

KorGgenT commented Nov 8, 2021

so before i do an extensive review, i have a requirement: you need to do # 4 & # 5 in a separate pr. i will not review a PR that both makes changes and also shuffles things around in files. especially because this is nearly a thousand lines to look at and review.

@Terrorforge
Copy link
Contributor Author

I can do that. I'll probably revert this one to just be the dragon fixes, then I'll make a new one that does the moving and renaming.

Should I wait until the first one merges, or do I fork this branch or something?

@KorGgenT
Copy link
Member

KorGgenT commented Nov 8, 2021

Should I wait until the first one merges, or do I fork this branch or something?

that is entirely up to you.

@Terrorforge
Copy link
Contributor Author

I've submitted the fixes as a separate PR, #52941. I'll be doing the reorganizing once that's in. I'm closing this one.

GuardianDll added a commit to GuardianDll/Cataclysm-DDA that referenced this pull request Jan 1, 2022
@ZhilkinSerg
Copy link
Contributor

Needs conflict resolution

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Mar 2, 2022
@ZhilkinSerg
Copy link
Contributor

Please resolve conflicts and either open as new PR or ping me to reopen current PR.

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 Mods: Magiclysm Anything to do with the Magiclysm mod stale Closed for lack of activity, but still valid.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants