refactor: move item groups to generic factories, move inline itemgroup loading to generic readers#9791
Open
WishDuck wants to merge 6 commits into
Open
refactor: move item groups to generic factories, move inline itemgroup loading to generic readers#9791WishDuck wants to merge 6 commits into
WishDuck wants to merge 6 commits into
Conversation
…p loading to generic readers
d5e7f6a to
0450982
Compare
Contributor
|
Autofix has formatted code style violation in this PR. I edit commits locally (e.g: git, github desktop) and want to keep autofix
I do not want the automated commit
If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT. |
422c1a3 to
d18e299
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of change (The Why)
Item groups defied every law of the universe
Only item_group_id::is_valid was implemented, and it was rarely used
Item groups were shoved into item factories
Item groups could not copy from each other
#9755 showed that there was issues with inline itemgroup loading.
But failed to recognize that this also applies to professions, monster weapons, construction, vehicle bash results and mapdata locations. And instead implemented a single solution instead of an overarching one.
It also failed to support deleting specific things / references from monsters if neccesary
Describe the solution (The How)
Move item groups to generic factories, adding required default copy from and reset variable support to generic factories
Made optional return a bool if a value was applied, as itemgroup loading depended on that logic and it would be useful elsewhere
Shift item groups from using unique pointers to shared pointers, as item group modification is limited to adding and removing these pointers, thus it is compatible.
Add
itemgroup_readergeneric reader for usage in loading inline itemgroups. It supports copy from along with all standard methods of itemgroup inheritancerevert feat: make death drops extendable #9755 as it is not necessary to destandardize from default itemgroup stuff and this allows default extend and standard itemgroup delete by default ( I am willing to budge on this, but there is a caviat, it will stick out as a completely different way to define nested itemgroups )
Describe alternatives you've considered
Split this into two PRs
Keep most of the old loading as halfway through the refactor I found moving from unique to shared pointers was enough
Testing
It loads the C.R.I.T. extend and delete stuff right
New tests pass which show all the inline inheritance stuff working ( individual types do not need to be tested as they all use the same runner )
Checklist
Mandatory
closes #1234in Summary of the PR so it can be closed automatically.Optional
docs/folder.Translations
Build Artifacts
PR build for commit 4c2dd14 (fix inline defaults and cleanup) on 2026-07-04 02:41:04