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

Make it More Automatic #18

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

Hamunii
Copy link

@Hamunii Hamunii commented May 25, 2024

Copy link
Owner

@harbingerofme harbingerofme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some branding stuff we need to decide: is the entire project gonna be renamed?
I'll have a more thorough look later.

README.md Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should prob fix this up a bit before merging

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are things I also wanna do like some minor optimizations that are super easy to do (which came from very minor performance regressions from very quick fixes because the thing was broken lol).

And making sure it won't break from for example a mod named "Assembly-CSharp" which I think could happen (which is kinda horrible), but it's not a problem as long as no one knows about it. But it definitely needs to be done

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yeah anyways about the branding, it could be somewhat confusing in some cases when if you don't know which version of HookGenPatcher is used, which is mostly a problem because there are many uploads of HookGenPatcher by different people and we can't really just update them like that.

List<string> paths = new();
Directory.GetFiles(Paths.ManagedPath, "*.dll", SearchOption.AllDirectories).ToList().ForEach(paths.Add);
Directory.GetFiles(Paths.BepInExAssemblyDirectory, "*.dll", SearchOption.AllDirectories).ToList().ForEach(paths.Add);
Directory.GetFiles(Paths.PatcherPluginPath, "*.dll", SearchOption.AllDirectories).ToList().ForEach(paths.Add);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patchers should be unloaded during runtime, so never accept mmhook files for them

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see. Yeah maybe it doesn't really make too much sense to have it, but then again there could be some rare case where it could be useful, but yeah I don't know if that case will ever come

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyways about this, I've thought about it, and I think this functionality should stay because the whole point of this software is to generate MMHOOK files for the user. And we can't know what the user (modder) is doing and as I said before, it's entirely possible they want to patch a patcher from their plugin, as we also can't know what kind of patchers there might be.

I don't like placing restrictions on what kind of assemblies can be MMHOOK'd if there is no technical reason for it. Well, I don't actually know if there is a technical reason for this, so do correct me if there is.

// TODO: This is horribly unoptimized, we already get MMHOOK file's path in GetPlugins. Fix this.
var thisMMHOOKPath = mmHookFiles.FirstOrDefault(filePath => Path.GetFileNameWithoutExtension(filePath).EndsWith(cachedAssemblyInfo.GUID));
if (thisMMHOOKPath is null){
Logger.LogError($"[{nameof(IsPluginInfoUpToDate)}] Plugin {cachedAssemblyInfo.GUID}'s MMHOOK was found yet it couldn't be found, this shouldn't be possible.");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can happen when optional dependencies are installed/uninstalled

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to sure on build stuff yet. Rather do automated github releases only

@harbingerofme
Copy link
Owner

Haven't forgotten tihs PR, jsut a bunch going on atm

@Hamunii
Copy link
Author

Hamunii commented Jun 1, 2024

Yeah that's completely fine, I'm also a bit too busy to work on this right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants