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

Change: Remove duplicate fields #1425

Closed
wants to merge 14 commits into from
Closed

Conversation

Stubbjax
Copy link
Collaborator

@Stubbjax Stubbjax commented Nov 5, 2022

Removed duplicate fields from various objects / definitions. Only assessed parent fields, not children. The former field was always removed, unless the value was identical, in which case the field with the worse / less consistent formatting was removed.

Note: Did not address the duplicate ExperienceValue fields of SpectreGunship, SpectreGunship1, SpectreGunship2, SpectreGunship3 and A10Thunderbolt. Also did not address duplicate RadarPriority fields of AngryMobNexus. The original values for the respective fields look more correct than the duplicate / used values and are better addressed separately.

@Stubbjax Stubbjax added the Minor Severity: Minor < Major < Critical < Blocker label Nov 5, 2022
@commy2
Copy link
Collaborator

commy2 commented Nov 5, 2022

I know it is extra work, but can we split stuff like this into individual PRs? The changes are all unrelated to each other, e.g. there is no connection between the Spectre armor values vs Flashbangs and Black Lotus' command set. Would make it easier to review and look up stuff later. Also, some values are not merely duplicates, but different values that overwrite each other. This still documents intend that is lost by just deleting the lines. Are we sure some tokens can't be used twice? E.g. research sound of Angry Mob does contain a voice line and a cheer. What's up with the first effect?

@Stubbjax
Copy link
Collaborator Author

Stubbjax commented Nov 5, 2022

I know it is extra work, but can we split stuff like this into individual PRs? The changes are all unrelated to each other, e.g. there is the connection between the Spectre armor values vs Flashbangs and Black Lotus' command set. Would make it easier to review and look up stuff later.

Sure, though I'm not sure it'd make it easier as each commit can be assessed just as easily as separate pull requests.

Also, some values are not merely duplicates, but different values that overwrite each other. This still documents intend that is lost by just deleting the lines. Are we sure some tokens can't be used twice? E.g. research sound of Angry Mob does contain a voice line and a cheer. What's up with the first effect?

I tested most of them and they are straight-up overridden. I looked at the angry mob upgrade sound and it is silent - the audio it references does not exist.

@xezon
Copy link
Collaborator

xezon commented Nov 5, 2022

The change can be committed as "Rebase and Merge", then all commits will be preserved on Main. (#1425) must be appended to all commit descriptions in that case.

@xezon
Copy link
Collaborator

xezon commented Nov 5, 2022

How did you search and find these duplicates? With a script?

@commy2
Copy link
Collaborator

commy2 commented Nov 5, 2022

the audio it references does not exist.

Okay, then it can be removed without replacement.

Same for true duplicate entries (twice the same value). Can be removed as is.

But when two conflicting values are used anywhere, then maybe it should be noted what the other value was, even if it did not apply.

How did you search and find these duplicates? With a script?

Searching these by script would be very helpful. I don't think the changes in this PR are anywhere near complete. Another reason it should be split up imo.

@Stubbjax
Copy link
Collaborator Author

Stubbjax commented Nov 5, 2022

How did you search and find these duplicates? With a script?

Yep, with a script.

Searching these by script would be very helpful. I don't think the changes in this PR are anywhere near complete. Another reason it should be split up imo.

It is true that there are likely to be more. I will split the changes into multiple PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minor Severity: Minor < Major < Critical < Blocker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants