-
Notifications
You must be signed in to change notification settings - Fork 358
feat(Lua): NotifyOnNewObject for unloaded classes #1134
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
base: main
Are you sure you want to change the base?
Conversation
|
Note that I was unable to find any classes that didn't follow the |
|
Would it be appropriate to do this for functions for registration in the same PR? I guess functions are a hotter path to have to do string/fname comparisons constantly |
I consider it out of scope, and will just delay the PR getting merged. But regardless, here are some notes on RegisterHook if we were to store names instead of immediately hooking. We would lose the ability to determine whether the function is native or not, which means we wouldn't know how to hook it. Native functions use While BP functions go through ProcessInternal or ProcessLocalScriptFunction. |
I think we can also mark the BP funcs native later on so they still go through our hook first rather than dealing with distinguishing |
| attempt_to_call_callback(object_class); | ||
| for (const auto comparison_class : Unreal::TSuperStructRange(object_class)) | ||
| { | ||
| attempt_to_call_callback(comparison_class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original code also followed this pattern so doesn't have to be fixed here I guess, but currently it's doing
for each super class -> for each callback. It would be less iterations to do for each callback -> check against each super class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I haven't done any performance testing here, have you ?
I'm going by the knowledge that iterating linked lists is slower than iterating vectors, so therefore it makes a certain amount of sense to iterate the linked list just once and the vectors multiple times.
But as I said, I haven't done any performance testing for this particular use-case, and I'd expect this to not be the case with more and more callbacks, performance testing is needed to figure out which is faster at a reasonable number of callbacks.
Description
This PR makes
NotifyOnNewObjectwork for unloaded classes.Note that this PR touches performance sensitive code, that can affect asset streaming or loading times.
I haven't personally noticed any slowdowns as a result of these changes, but be on the lookout for that.
Type of change
How has this been tested?
In Palworld, with the following script:
Checklist
Screenshots
Additional context