Skip to content

Conversation

@Kein
Copy link
Contributor

@Kein Kein commented May 22, 2025

We shouldn't do anything if there is no PAK mods.

@narknon
Copy link
Collaborator

narknon commented May 24, 2025

I think this looks good but would like @UE4SS or someone that works with Lua more to look at it.

Copy link
Collaborator

@UE4SS UE4SS left a comment

Choose a reason for hiding this comment

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

What a mess of a PR.

Parenthesis added to a lot of if-statements where none is needed.

Re-organizing the file in a commit that also has logic changes, causing the diff to be unnecessarily large which in turn requires a full re-review of the moved code to figure out if the code was actually changed (it was) or just moved.

For future PRs, if you cannot avoid reorganizing code, please do so in a separate commit to make it easier to review.

@Kein
Copy link
Contributor Author

Kein commented May 25, 2025

What a mess of a PR.

Then simply close it, why are you wasting your time on complaining and/or wasting my time as well.

@UE4SS
Copy link
Collaborator

UE4SS commented May 25, 2025

What a mess of a PR.

Then simply close it, why are you wasting your time on complaining and/or wasting my time as well.

How is it a waste ?
I provided clear feedback.
If you choose to not take that feedback on, and not even attempt to counter any of it, then that's your problem and you're the one wasting everyones time.

UE4SS
UE4SS previously approved these changes May 25, 2025
@narknon
Copy link
Collaborator

narknon commented May 25, 2025

I'd like to cleanup some of the typos/comments before merge, but I can do that if Kein doesn't want to.

@Kein
Copy link
Contributor Author

Kein commented May 25, 2025

I can do that but ideally I also would like to either merge BPML GenericFunctions to modloader unit or export BPML GenericFunctions logic into a shared helpers and use them conditionally in BPModLoader.

My main concern is pointless 2 events that still being registered when no supplementary blueprint mods exist. I'm bad at reading C source but my understanding the match for event name is checked every single time ANY event execution call is, well, called.

P.S. I know that any BP can use these events not just modactors

@narknon
Copy link
Collaborator

narknon commented May 25, 2025

Yeah, BP hooks are rough on performance, though it's just an int comparison. People do use the print even without using our modloader so we can't really just make it suddenly not work, but we have other hooks that seem to hook even when they're not used, which is a mess.

@Kein
Copy link
Contributor Author

Kein commented May 25, 2025

May be we can move to a shared instance that can be required by the mods and then the mods can request a hook/BP modloader framework init with simple API/fun call? Under the hood, the API will check if hooks were created and if not, they will be. This way we dont even need to create anything pre-emptively, so if no mods for the game plan to use this functionality - none created.

The only disadvantage is that mods need to opt-in now

@Kein
Copy link
Contributor Author

Kein commented Jun 9, 2025

As a proposal, I also changed the reload hotkey to ALT+INS.
This has been something I wanted to propose from the get go but it slipped my mind. Too many games and too many users use INS either in-game (actual bind or a debug bind) or outside (overlay, etc). Reloading properly loaded mods by mistake in-game can cause catastrophic issues.

@Xebeth
Copy link
Contributor

Xebeth commented Jun 9, 2025

I second that. Usually INS is my Reshade key.

@UE4SS
Copy link
Collaborator

UE4SS commented Jun 9, 2025

As a proposal, I also changed the reload hotkey to ALT+INS. This has been something I wanted to propose from the get go but it slipped my mind. Too many games and too many users use INS either in-game (actual bind or a debug bind) or outside (overlay, etc). Reloading properly loaded mods by mistake in-game can cause catastrophic issues.

This should be added to the changelog.

@Kein
Copy link
Contributor Author

Kein commented Jun 9, 2025

Changelog where exactly? The CHANGELOG file?

@UE4SS
Copy link
Collaborator

UE4SS commented Jun 10, 2025

Changelog where exactly? The CHANGELOG file?

assets/Changelog.md

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.

4 participants