-
Notifications
You must be signed in to change notification settings - Fork 377
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
Changed how data pushed to lua is validated to use weak_ptr #469
Conversation
…lementation * Removed additional indirection when pushing 64 bit integers and ObjectGuids
LuaEngine.cpp
Outdated
if (event_level == 0) | ||
InvalidateObjects(); |
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.
We may still want to have the old system to invalidate pushed objects at end of an event stack.
Or at some specific event, like at start of map updates starting.
One of the issues is that if you have an aura for example aura = player:GetAura(spellid)
, then that aura will be valid until the aura object is destroyed.
But as I understand, aura can be transferred to other maps with a player.
If we dont use some specific event to always invalidate objects, then it can happen that an aura or other object is transferred to another map and can be accessed from two lua states at the same time during map update events when we run multi-state mode eluna.
Any thoughts on this?
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.
Actually, you don't see this in the PR but aura handle is not invalidated when its destructor runs (which is delayed a bit, first it gets added to m_removedAuras list) but immediately when it is unapplied from unit (see TrinityCore/TrinityCore@a79b42b#diff-f3fb604c220161af7953902aecc9586825b8193413ab94fdeea4de5665d7a393R622)
But you are right about moving to another map - I will need to think about it since I was planning to use this outside of lua too
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.
Luckily aura already has another reference it can additionally check, its WorldObject owner - so this is what I came up with e676a56
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.
There are also Loot functions proposed here.
I wonder how cases like this should be handled where there is a new type exposed that does not necessarily have the unique_trackable_ptr.
I guess introducing new type would always require modifying the cores first to introduce the tracking and then the type can be exposed after.
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'm starting to think its would be better to only allow preserving references for longer for some types, not all of them (using the old callbackid for the ones we want to exclude)
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.
Currently the primary objects where storing the GUID and fetching the object is the norm is; players, creatures (units) and gameobjects. I'm guessing we can start with those at the very least and expand where it makes sense.
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.
Should be addressed by 39eed65
e676a56
to
bd6de45
Compare
# Conflicts: # ElunaTemplate.h # LuaFunctions.cpp # TrinityCore/PlayerMethods.h
While working on something else I got reminded of this PR. Thoughts on wrapping the proposed changes in TC ifdefs to preserve cmangos functionality for now, and then remove the legacy code if/when a similar implementation is added to the other emulators? |
This should make cmangos build fine until a future PR can be done
I'll see if I can get a cmangos server set up to test some time this weekend, unless someone beats me to it edit: |
Makes use of TrinityCore/TrinityCore@a79b42b
Also removed additional indirection when pushing 64 bit integers and ObjectGuids
This allows storing direct object references in scripts
Player handles are intentionally invalidated on cross map teleports
Result:
PR is marked as draft because map create/destroy hooks currently don't work (fix needed in main TC repo that changes where they are called)Other emus won't currently compile - I plan to submit a PR to CMangos with the same commit that was pushed to TC (in the worst case that nobody is interested in adopting that code it would have to be instead done by forks that maintain Eluna integration)