Fix tbl merge bug and add generic file merging #37
Conversation
DeathChaos25
commented
Oct 14, 2024
DeathChaos25
commented
- Fixed TBL Merging bug related to expanded TBL files
- Added generic file merging for a bunch of files for P3P, P4G and P5R
- Add testing units for new generic merging
- Fixed TBL Merging bug related to expanded TBL files - Added generic file merging for a bunch of files for P3P, P4G and P5R - Add testing units for new generic merging
DeathChaos25
changed the title
|
Just woke up, had a peek, will look more in depth at this later |
There was a problem hiding this comment.
Have a look at my comments. The one that concerns me and definitely needs to be addressed is my one on TblPatcherCommon.cs related to this change probably changing the order patches are applied, meaning priority isn't respected.
My other comments are kinda just nitpicky.
Persona.Merger.Common/Patching/Tbl/FieldResolvers/Generic/GenericPatcher.cs
Outdated
Show resolved
Hide resolved
|
|
||
| /// <summary> | ||
| /// Applies a given list of table patches to the current file's TBL segments. | ||
| /// </summary> | ||
| internal static unsafe void ApplyPatch(List<TblPatch> patches, int x, Memory<byte>[] segments) | ||
| { | ||
| patches.Sort((a, b) => a.SegmentDiffs[x].LengthAfterPatch.CompareTo(b.SegmentDiffs[x].LengthAfterPatch)); |
There was a problem hiding this comment.
Won't this change the priority of patches based on their length? For example:
- Patch A is 20 bytes long and changes byte 5 to FF
- Patch B is 10 bytes long and changes byte 5 to CC
With this I'm pretty sure Patch A will always be applied first as it's longer. So, byte 5 will always be CC regardless of what order the mods are in (I'm assume a later patch takes priority for the example but it doesn't really matter as either way you're altering the order to be different to what the user set).
If patches actually need to be applied in order of length to work correctly I think you need to come up with something that doesn't reorder them. Maybe if you append 0s to the end of segments that are too short so they are all the same length? (Not really sure how this stuff actually works, that might not work at all)
There was a problem hiding this comment.
You should also add some unit tests to prove that this is working correctly (whether it already is correct or needs to change). Just something simple like my example where you have two patches of different lengths and check that the order you add them is respected properly.
There was a problem hiding this comment.
While it is true that priority isn't respected in regards to files that are larger than the original, this will not affect most scenarios with merging where all files are the same length.
This is unfortunately the only viable solution to fix the problem with merging expanded files from within Persona Essentials itself.
No other code change seems to fix the underlying problem, such as forcing an arbitrary LEN value that is bigger than expected, or even manually walking through the list of patches, finding the biggest len value, and then forcing that value to be the only len value ever used.
The problem seems to lie in the patcher code itself (which is not rewriteable from within Persona Essentials) where each "patch" copes the original file and then uses its length and applies the "patches" onto it, and if at any point a patch is shorter than the previous one, the entirety of the data that exists beyond the original LEN is lost in its totality.
This is the only approach (from within Persona Essentials) that will avoid this behavior, any proper fix would need to happen within the original patcher source.
Re: Unit testing, there is a new unit test added that specifically tests both generic patching and expanded file patching.
There was a problem hiding this comment.
@Sewer56 You can decide whether having the order patches apply change is acceptable as a temporary fix.
Personally I think it'd be fine as I can't really imagine the scenario I described happening often. If someone is using an expanded TBL I would expect that they'd be adding stuff into that expanded section rather than the original part.
Fixing the game crashing if mods have tables of different sizes is also much more important than the potential conflicts the solution would cause imo.
There was a problem hiding this comment.
In General
I'm not sure we can do this here. The problem is; this isn't really a 'temporary fix'. This is a hack, with side effects.
There's a core expectation from end users being what worked today will work tomorrow, provided the game version's fixed. Even if it's 0.1% of people (i.e. an edge case), modders will make mods expecting this behaviour, and end users their mod setups around it too.
The problem is, if you make a temporary solution, and then change how that solution works down the road, you will break an end user's setup. As a more 'general' example, an end user may upgrade essentials, load a save, and their game may wind up broken because the item order changed, and that doesn't match whatever is serialised in a save game; the game now crashes to desktop. In this specific case, the side effect isn't that serious; but the point still applies.
Once you add a certain behaviour to a core mod everyone requires like Essentials, this behaviour essentially becomes a part of the API; even if said behaviour is considered to be a side effect. Therefore it's inevitable that you get things 'right' the first time, and never break existing behaviour.
In some cases breaking changes are unavoidable; e.g. if the code was not correctly written in the first place; however the most important thing is to avoid doing it incorrectly if possible in the first place.
I am a bit more strict about this sort of stuff than I used to. Reloaded-II doesn't have the right facilities for manual version control; for example you can't have 2 profiles for a game, each using different versions of a given mod. It's all bleeding edge/rolling release.
The Side Effects
I mentioned that the proposed approach has side effects here, but what do I mean? It mostly has to do with load ordering.
- There's being an assumption made here (in the comments and code) that patches which extend the TBL files don't modify any base game entries.
What I mean is that the proposal here is to move the patches that extend the TBLs to the end of the patch set. However, if these same TBLs also modify the base entries, then the mod load order set by the user is not respected. Those that extend the TBLs will always gain priority.
- The sort used is not a stable sort.
Sorting the elements such that items with a longer TBL length are placed last with the current code means that the order of the elements where the TBL length is equal is not guaranteed to be preserved, since the sort is unstable.
Searching for a Better Approach
In any case, there is a distinct set of challenges involved in adding additional entries. Suppose the following case.
Base file has 10 entries.
ModA adds 5 entries to the end.
ModB adds 5 entries to the end.
Case 1
If the new entries in ModA and ModB are completely unique; they should be appended to the end, i.e. there should be 10 new entries total. (5 per mod).
Case 2
Mod author of ModB extended the file from ModA, so there are some shared additional entries, but not all.
In this case, only the unique extra entries should be added.
Case 3
Mod author of ModB modified the extension from ModA
In this case, ModB wishes to amend an entry from ModA.
Other Considerations
Order of appends into an existing file should follow mod load order as a general rule of thumb, in the case it matters for a given specific use case.
If any file refers to an entry in another file by index, those indexes also needs to be fixed up.
What do we do with regards to these.
Case 3
Case 3 is the very difficult case. It's hard to determine if a different entry is an edit to an existing one, or a fresh entry in most cases; without human intervention. I think I'd recommend leaving this out for the time being. Reloaded3 spec does have a mechanism for marking one mod as a replacement for another one, and you are supposed to make mods as minimal/modular as possible. So I think using that mechanism would be the suitable approach in the future, since you're modding a mod at that point. That can be left out for now.
Remaining Cases
Cases 1 and 2 are doable, but you probably want a separate diffing mechanism here; the diff library is meant for diffing data where you know 2 sets of bytes are supposed to represent the same items (base game items). The fact the base library can be/was used to extend here is just a happy accident; an accident done to quickly satisfy a request. Anyway here you don't know if they do indeed represent the same items. In practice, extended data generally does not.
To satisfy Cases 1 and 2, you really want to only do a diff on the base game entries; as they can be assumed to represent the same data. For the remaining entries, you want to use a HashSet. Hash the new items beyond the end of the original file and add each new encountered item if it's not already in the HashSet. This way you only get uniques.
There was a problem hiding this comment.
If I understand your proposed solution correctly it is going to cause problems.
For example we have:
- A TBL file of items with 100 entries in the vanilla one
- Mod A are adds item "ABC" as entry 101
- Mod B adds item "FFF" as entry 101 as well
How I think you would expect this to work is: we get a merged TBL with 102 entries. 101 is the from the first mod and 102 is from the second.
If that is what you intend it's going to cause so much confusion. Stuff is done by index in game so you really can't be changing the index of stuff like this.
Say I switch around my mod order. Previously I had 10 "ABC" in my inventory and 50 "FFF", now I have 50 "ABC" and 10 "FFF". Obviously this should never happen.
If two mods edit the entry at the same index that entry should be merged whether it's from the vanilla table or an expanded version. If they're doing this you have to assume it's either intentional and they want to be merged or it's a bug and it's then up to the creators to change their mods to use different indexes.
There was a problem hiding this comment.
Stuff is done by index in game so you really can't be changing the index of stuff like this.
I anticipated this, and therefore said:
If any file refers to an entry in another file by index, those indexes also needs to be fixed up.
I simply left that out as a separate discussion topic for the future, as I don't know the specifics of the saves and table formats here. (Been forever since I looked)
And it's then up to the creators to change their mods to use different indexes.
This is the main problem at the crux of all of this; how do you guarantee unique indices?
Can mod authors even assign indices? I imagine most game code would be something like 'use 101th item' when deserializing a save or similar, rather than 'use item with ID 101'.
In such case, there would be no specific way to assign an ID to a given item.
In any case, the expectation of an end user is they add 2 mods, and both mods add some form of item. (Table entry).
They don't care about the specifics, they just want something that 'works'. I'm not sure if such a thing is possible myself if it's all index based; and not by unique ID.
If it were ID based, you could maybe hash the entry data itself, and assign ID based on hash; it's not ideal if the ID range were 16-bit or smaller, but at least you'd know if there are collisions at generation time.
There was a problem hiding this comment.
The ID of an item, for example, is based solely on its position in the table. The first entry is always ID 0, they don't have something in the entry that says what the ID is. I'm fairly sure this applies to everything, items are just an easy example.
What you're describing sounds nice but is adding an unreasonable amount of complexity. If people are binary editing tables then they have to deal with their limitations such as needing to check that they aren't using the same indexes as other mods.
To do what you describe would require a large framework mod. For example costume framework exists and basically does this for in game outfits, you give it a model and it just adds it to your inventory display. There's no messing around with binary data and indexes.
Doing something like that is way out of Persona Essentials' scope of merging and loading files. It requires hooking game code and would need different implementations per game. Maybe you could get away with just changing files but even if you could you'd still be adding huge complexity and needing to do stuff like editing save files which definitely isn't generic.
There was a problem hiding this comment.
then they have to deal with their limitations such as needing to check that they aren't using the same indexes as other mods.
Aren't they using the same index the moment they add a single new item to the table by definition?
I'm not sure why you would check at that point; conflict is unavoidable.
Doing something like that is way out of Persona Essentials' scope of merging and loading files.
I agree, I've expressed my dissatisfaction at PEssentials being too monolithic to begin with, and have stated this to be a mistake.
A single mod should only serve one function; merging TBL files should be within a separate mod ideally. And it should only target one game. That mod could then become the framework eventually. Essentials shouldn't be merging stuff like 'Skip Intro' and 'Merge Files' in a single mod; because then you'd have to make an API to disable individual parts and the whole thing would become a mess.
In any case, even if ID resolution is out of scope for the time being; the load order of the files must still be respected one way or another. The current code reorders items, breaking apply order, and then performs an unstable sort; which can re-order unrelated elements. (As per my original comment).
If we're to reduce it to a simple model where we don't care about any of these problems for now, then I'd recommend the original suggestion from @AnimatedSwine37 in the first post. Pad the files to match the longest, and then diff against that. This should result in a situation where all items beyond the original item count replace each other; unless padding byte (00 / FF / whatever else) is encountered.
If we were to apply the example from #37 (comment), then only one new item ABC would exist (rather than both). If the user swaps the mods in load order, the item could change from ABC to FFF.
I'm not sure which behaviour is more desirable between being additive and replacing. Replacing reduces the chance of crash to desktop slightly on mod deletion/removal but may not match the user's expectation, of I enable 2 mods and I get 2 new items. I think additive is better UX; but it's ultimately up to the community to decide. Ask in Discord?
In any case, it should be common knowledge that applying inventory modifying mods in the middle of a playthrough is unsafe; that's pretty much the case with all RPGs in practice; unless measures are specifically put in place.
There was a problem hiding this comment.
This is fixed with my change in 69d3215.
Instead of the patched data completely overwriting the previous version of the segment the patcher writes onto the previous version so if it writes less than the length of it those remaining bytes are just left unchanged.
