-
Notifications
You must be signed in to change notification settings - Fork 2
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
Enhance patching feature #161
Comments
The formatting changes unfortunately make a reading this patch extremely difficult, but for now: Why did you remove the old patches and patches.json loading entirely? Internet patches should just extend the currently loaded ones. (don't break existing behavior, everybody isn't playing the newest games) |
|
I agree that the built in patches.json should not be removed. Plenty of people rely on this especially for offline cab use. Have you also tested if a patches.json file (using existing format), placed next to spicecfg.exe also continues to be loaded & parsed correctly? That's another feature that people use today. Especially the game detection logic - we should be really careful not to randomly break existing installs that rely on datecode-based matching. Strongly suggest reverting any formatting changes unrelated to the feature. It pollutes source control blame history & makes things difficult to diff during reviews. |
reading from local files can be added back. but as said in the previous response, it would not work with the built in ancient patches. it could be adapted if pe header values are identified for each version. the whole purpose of switching from datecode matching to pe header identification is not cosmetic, but for actual version detection rather than having to rely on ea3 config and such, but if that's "randomly breaking things" or "unrelated" for you, i have nothing to say. i'm closing this issue since absolutely no one wants change. |
for the whole time i thought you guys were refering to my changes on the json formatting instead of the code indentation at unnecessary places. sorry for the misunderstanding and the response earlier. i can fix these no problem. about the built in patches, as said before i would need pe values for all versions in there or i could just add in the old json logic just for the built in patches. so its either someone provides me the necessary dll files or we store two json logics in the code |
Yep, I was referring to code style changes (such as indentation) and not the JSON changes. Apologies as I wasn't entirely clear. I still think this is a cool feature, to move the patches out of spice but to external services that can be updated more frequently. Please do keep the old logic in tact, for both datecode matching and parsing of the old JSON format, and keeping the built in patches as is. This new JSON format and PE-header based matching should be added as separate logic. It's OK if we have some duplicated code. If you have a sample of the new JSON format, it would be appreciated for easier understanding of the code. If there is a conflict; e.g., if a game matches based both PE-header based matching and the datecode-based matching - we can prefer the new patches, or perhaps they can be smartly merged somehow (although that might be messy). Another interesting point for testing: Spice Companion actually also ships with patches.json, so that you can remotely apply patches. We would want to ensure that this feature continues to work (as the receiving end / API server anyway, the clients are assumed to be dead and no modifications made for years). Due to personal reasons I have not had all that much time for this project. Since this is a significant contribution, it would be really appreciated if others can help with code review, feedback, and testing. |
git diff example json that utilizes union type |
I meant so the user can supply a full URL to be formatted, with retrieve.php as an example so a "URL format" doesn't need to exist.
The concern is if the patch server is offline, DNS trouble, or the user is offline, it shouldn't infinitely wait trying to retrieve patches. |
Can the new JSON patch format contain the DLL identifier string? As in,
your example looks like this:
Can we have this instead?
This way, we are not relying on the file name (or URL). Why is this needed? It's because it's legal to have a giant patches.json file locally that has a mix of patches that apply to the current game version, and patches that do not apply. Therefore, the identification needs to be done at per-patch level, not per-file level. It would also help us to distinguish between old and new versions of the patch format. |
One more point to consider: I understand the convenience of this and many people want it, no disagreeing there.... But one concern I have is that this is technically one giant (intentional) Remote Code Execution. If someone compromises your patcher site, they can push bad code to your DLL, and people will unknowingly run them with admin privileges. Obviously, they can still do this today, just with extra steps - someone can still hijack your patcher, inject bad code into the page, and let people download bad DLLs - but it shifts the responsibility to spice. I'm not saying we need to stop this, but we might need to put some safeguards in place. I can't think of anything other than a big red warning label at the moment, but feedback from others is appreciated. |
|
Well, at least with patches.json, the user had to download a file, and with spice API, it required a password. But I agree, I don't think there is much that can be done. |
I think the original maintainer had similar concerns with integrating automatic updates / checking. Maybe a warning or confirmation + patch view button (ImGui hex editor) for patches longer than x bytes / x offsets? Most are usually a couple bytes in one or two offsets, then there's reflec groovin upper / DDR X3 song unlocks... and very partial 'translations'... |
SDVX Plus patches tend to be very long and complicated, but also widely used at the same time. |
My suggestion for the UI would be to:
We should not be automatically pulling patches from the URL every time and applying them on launch. I would go as far as not saving the URL in the config file and instead require the user to enter it every time they want to import. Introducing additional manual steps as above serves sufficient warning for the user what is going on, and provide an opportunity for more advanced users to inspect the patches file before letting them be applied to the game before running. I understand we want things to "just work", but I also don't want a situation where someone malicious creates a legitimate patcher website initially & convince many people to set it up as auto-patching, and then later change the patches to distribute malware, and be in the middle of it as a distributor. This would get us a lot of bad press in the community & and risk getting flagged by anti-virus software (not just by accident but added explicitly as malware medium). Last thing we want is another moral panic over how spice is a crpto miner... I have some mixed feelings about the Hard Apply button as well... one of the original stated goals of spicetools is to run on clean data. It feels like it goes against that. If the runtime memory patching works, why do we need to write to the file? At least a confirmation dialog would be nice to have, explaining that original DLLs will be overwritten with no backups made. You'd be surprised how many people never make any backups of their game data ever, and re-download the whole thing if they mess up. |
Changes
Q&A
Ideas/Stuff to implement
any objections? |
Sounds good to me. One question though, what is the reason for cab owners not applying patches at runtime? |
this seems like more of a preference topic they seem to find the requirement of plugging in a mouse/keyboard to the cabinet to set patches rather overwhelming. i also hear the "cab users might not use spice" excuse but i think this is too subjective and i thought you could think "then they shouldn't use spice at all". another thing is for example, for games like jubeat that run on XP, they will not be able to operate with spice2x at all. for now i'm not really sure what to do with this functionality so i'll wait for more opinions. also more clarification on the new implementations: there will be two extra folders on the spice directory. "remotepatches" for well, to store remote patches from given URL. and user-specified local folder. remotepatches will only process "_imported" json files, priority being on the "patches_imported.json". if "patches_imported.json" doesn't exist or doesn't have any corresponding patches to the loaded module, spice will attempt looking for "{identifier}_imported.json" instead. as for a local folder, spice will only process non-imported files with the priority being on "patches.json" then "{identifier}.json". this logic prevents conflict if the user ever decides to put a json file in the folder it doesn't belong to. to keep track of which folder to use when loading patches, a boolean entry will be added to the patch config json. this only changes if the user changes the type of set patch source. |
So we have
Is that correct? I think we can drop the _imported from the file names, if they're going to be in a separate directory. By patch config json you mean |
yes that's the json i was referring to. and actually now that i think about it, if we completely drop the "imported" naming on json files we can just use a single folder for every json and completely forget about the url/local classification. |
Changes
git diff: |
Do you have a URL that can be used to test? Couple issues I found:
Built-in patch database does seem to load properly. |
|
Just want to say up top that your contribution & patience through the review process is appreciated.
I have not been able to access
I already explained this previously, but this isn't really up for a debate. It's easy to introduce breaking changes as a contributor, but as a maintainer I must avoid it at all costs. This may be an obvious thing to state, but as an open source project, the burden is on the maintainers to maintain submitted code, forever. We already have people using patches.json written against the existing format. Some people rely on this to boot their game, or at least get it to a playable state. If they upgrade to a version of spice that doesn't support this existing patches.json file, their games would stop working, or behave differently. Most likely this will result in issues being filed here, or people asking in various discord servers why their game stopped working. The way to "fix" it would be to:
There are also patcher websites that let users download .json files, again, using the existing format. All of that would just stop working. Sure, they can also start adopting the new JSON format, but now they have to tell everyone to upgrade spice2x first before they can use the new JSON files. Again, this is unnecessary burden on other to provide (free) troubleshooting & support for others. Your code already has logic to parse the old JSON format ( Until this issue is resolved, I can't accept this change into a release. If you really feel strongly about not adding support for this, that's fine, just let us know, and we'll find someone to add that support back. Couple other things I want to mention:
|
In case it helps, I've uploaded some of the generated files from my conversion script for IIDX 30 final, which should work when using Here are direct links to the JSON files:
The patch filename is determined by: (using LDJ-003-2023090500 as an example)
Which is then concatenated to produce the final filename: |
alright i'm thinking of two ways to re-support datecode method:
i don't think people would mind moving their existing json to a new location as for documentating the logic, i think aixxe has it all covered on the comment above. let me know if another thing has to be addressed |
I don't agree with this. The old format JSON in the root should continue to work.
I agree that new patches should stay in the patches folder. Can we use old logic for \patches.json, and assume everything in patches folder uses the new logic? I don't think we should have a folder for legacy. If that's too confusing for users, maybe the folder should be called
Agreed. |
so you're telling me patches.json can be located near spice or modules folder but this json will be parsed with old logic. and new jsons with the pe stuff will be located in the patches folder. is it correct? |
This is where we currently check for patches.json, turns out there are three: patch_manager.cpp:427 // automatic patch file detection
std::filesystem::path autodetect_paths[] {
"patches.json",
MODULE_PATH / "patches.json",
std::filesystem::path("..") / "patches.json",
}; and new patches can be in the new patches folder (or pe_patches folder if you want to differentiate) |
the priority has been changed to:
git diff |
Thanks. I'm going to be making the following changes:
after some testing, will let you know here when it's integrated, and put out a release shortly. |
I'm making slight changes to preserve backwards compatibility: from std::filesystem::path LOCAL_PATCHES_JSON_PATH_OF("patches.json");
std::filesystem::path LOCAL_PATCHES_JSON_PATHS_NF[] {
"patches/patches.json",
MODULE_PATH / "patches.json",
std::filesystem::path("..") / "patches.json" }; to // possible locations of old format JSON patches
std::filesystem::path LOCAL_PATCHES_JSON_PATHS_OF[] = {
"patches.json",
MODULE_PATH / "patches.json",
std::filesystem::path("..") / "patches.json"
};
// possible locations of new format (PE header ID) JSON patches
std::filesystem::path LOCAL_PATCHES_JSON_PATHS_NF[] = {
"patches/patches.json"
}; |
Found another issue - when reading in a local patches.json using the old format, datecodes aren't being checked, so it loads in all the patches (as long as the three character gamecode matches). Working on adding this logic back in. |
One more issue - we're looking for |
Issues above are resolved now, but I found a few more issues that I need to work through:
I'm not sure how to fix this. I would have liked the UI to look like this:
but patchers right now assume one of the drop-down options contain the default option, which means the checkbox will be always selected - needs a bit more thought. I don't think solving this is high priority. Alternatively we could add a Reset button for non-default settings, and clicking on it reverts to what's in the DLL:
For example, I might want to have
Same as above. Patches in iidx29 should not be auto-applied to iidx30.
|
Here is the archive of the source file so far: (removed - see alpha release below) Issues #2-4 mentioned above need to be resolved before it gets integrated into a release. I don't have the time or energy to resolve all of them. |
It's not a high priority right now I think, but I tried writing mockup Wiki documentation for the new format. Some descriptions are missing, and perhaps some things are wrong or might still change, but I hope it would be a good starting point for when the feature is integrated into a release. Edit 2024-04-30_17:21; |
Here is an alpha / pre-release version:
Please report bugs in this thread. |
Slight tweaks, calling this r0001: https://github.com/spice2x/spice2x.github.io/releases/download/24-04-30/spice2x-24-04-30_r0001.zip
An example is hosted on |
@sp2xdev would you be willing to allow patch submissions to this repo / occasionally convert mainline bemanipatcher commits to the new format, or is that going to be purely for example only? |
Just an example, no plans to host and maintain patches. |
No problem. I've updated my mirror repository to use the new format. Briefly tested with r0001 and appears to be working. |
I think I found something which might be unexpected behaviour (r0001);
|
Thanks for reporting. Should be fixed here: https://github.com/spice2x/spice2x.github.io/releases/download/24-04-30/spice2x-24-05-01-r0002.zip also contains some minor UI / logging updates and code refactor. |
I've got two minor patches for the The first is more of a refactoring around the The second adds in PE identifier comparisons for the old Attached the combined |
I've integrated the first patch. For the second patch, what's the delta between |
That makes sense. It was a bit unintuitive that the file would have the same name but be parsed differently depending on which folder it's in. I've tried combining the two loading functions and haven't noticed any side-effects yet from some brief testing. The combined JSON from my previous comment loads as expected when placed in either Tried using the old |
r0003: https://github.com/spice2x/spice2x.github.io/releases/download/24-04-30/spice2x-24-05-02-r0003.zip
|
I put out a beta release on the site with these changes. Thanks everyone for contributing code, submitting suggestion, and reporting bugs. Especially @Elpisdev for patiently (and positively) responding to feedback. https://github.com/spice2x/spice2x.github.io/releases/tag/24-05-04 |
This is a pretty nifty feature, and I'm here to ask some questions about making bemanipatcher more compatible with it. Feel free to @ me in future patch conversations if bemanipatcher is making your life difficult :P
This is mostly due to Salim's PR causing a bunch of "hmmmmm" questions - it uses heuristics to try and extract the gamecode/datecode, and not having to do that would be beneficial. |
Correct.
Yes.
There isn't a 1:1 conversion right now, so it might not be possible to convert your entire site to the JSON format. For example, spice2x doesn't understand "number range" format used for some offset patches.
The new format introduces a hash generated using the PE header of the DLL. This allows DLLs to be detected better (e..g, if you have 003 DLL vs. 010 DLL for IIDX) without relying on datecode. This is optional though. You can still use the old datecode matching & allow the user to download a patches.json file. Users will need to somehow figure out what DLL they have, so that they can download the correct JSON file... which could be a bit confusing. |
Does it just ignore them, or does it fail to load/
Is the datecode/mcode required, or can I just generate patches and trust the user (ha)? This is an extreme edge case, but translating strings inside the .dll by adding a new section header will probably break the PE header hash. Would spice ever use bemanipatcher's "just throw every patch set at the wall until something sticks" approach? |
Just tried it and it just shows up in the UI as
For patches.json: you need the For files in the patches directory: as long as the filename is correct (e.g.,
To be accurate, it's not a hash of the whole header, it only looks at the PE timestamp and AddressOfEntryPoint. So even if you modify or add new sections, those won't change. |
Thanks for the clarifications. I think I can get away with a big refactor to add proper datecodes to all the existing patchers, but I definitely can't get copies of all the DLLs to make the hashed names, so it'll have to be a download button and trust the users. That's acceptable to me, so I'll continue working on my rewrite. |
Description of change
This update introduces new features and improvements that make the patch management system more user-friendly, flexible, and efficient. Users can now easily fetch patches from an URL, select union patches, and directly apply patches to the game files in the standalone version of the program.
Changed the way how game versions are identified using the PE header.
Added support for a new type of patch called "Union Patch", which allows selecting one patch from a group of patches.
Introduced the ability to specify a URL from where patches can be fetched, making it easier to keep patches up to date.
Added a "Hard Apply" button in the standalone version of the program, allowing users to directly write the selected patches to the game files. (Useful for cabinet users)
Removed Herobrine.
Testing
Tested with SDVX, IIDX, DDR, jubeat, POPN, DRS. Used my own URL to fetch patches.
Patch file
git diff
forsen.patch
The text was updated successfully, but these errors were encountered: