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

Changed how data pushed to lua is validated to use weak_ptr #469

Merged
merged 8 commits into from
Jul 13, 2024

Conversation

Shauren
Copy link
Contributor

@Shauren Shauren commented Mar 14, 2024

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

local test_stashed_player = nil

local function OnTextEmote(event, player, textEmote, emoteNum, guid)
    if test_stashed_player ~= nil then
        xpcall(
            function() test_stashed_player:SendBroadcastMessage("reuse stored player") end,
            function(err) player:SendBroadcastMessage(err) end
        )
    else 
        test_stashed_player = player
        test_stashed_player:SendBroadcastMessage("store player ".. tostring(player:GetGUID()))
    end
end

RegisterPlayerEvent(24, OnTextEmote)

Player handles are intentionally invalidated on cross map teleports

  1. login
  2. /lol
  3. /lol
  4. .tele dalaran
  5. .rec
  6. /lol
    Result:
    obraz

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)

…lementation

* Removed additional indirection when pushing 64 bit integers and ObjectGuids
LuaEngine.cpp Outdated
Comment on lines 1012 to 993
if (event_level == 0)
InvalidateObjects();
Copy link
Member

@Rochet2 Rochet2 Mar 14, 2024

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?

Copy link
Contributor Author

@Shauren Shauren Mar 14, 2024

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

Copy link
Contributor Author

@Shauren Shauren Mar 14, 2024

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

Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Member

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.

Copy link
Contributor Author

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

@Shauren Shauren force-pushed the use_weaks branch 2 times, most recently from e676a56 to bd6de45 Compare March 15, 2024 11:26
# Conflicts:
#	ElunaTemplate.h
#	LuaFunctions.cpp
#	TrinityCore/PlayerMethods.h
@Foereaper
Copy link
Member

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?

@Shauren
Copy link
Contributor Author

Shauren commented Jul 12, 2024

Untested: https://github.com/Shauren/mangos-wotlk/commits/unique_trackable_ptr/

@Foereaper
Copy link
Member

Foereaper commented Jul 13, 2024

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:
Pushed a PR (Shauren/mangos-wotlk#1) to fix the missing GetWeakPtr functions. cmangos also does not have support for ToItem or ToWorldObject, so that would need to be added.

@Shauren Shauren marked this pull request as ready for review July 13, 2024 10:29
@Foereaper Foereaper merged commit f4a1203 into ElunaLuaEngine:master Jul 13, 2024
@Shauren Shauren deleted the use_weaks branch July 14, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants