Parse packet in lua feature #2978

nekiro · 2020-04-28T11:16:58Z

This pr allows you to parse packet in lua using creaturescripts interface.
I have included template creaturescript how to parse packet.
Registering a packet in lua means it will not be parsed in source code.

yamaken93 · 2020-04-28T11:55:45Z

There is a "major" mistake in this pr in the protocolgame code. The player methods and specially the creatureevents code should be called by the dispatcher thread, you are calling them from the main/asio thread = crash.

infister · 2020-04-28T12:39:15Z

There is a "major" mistake in this pr in the protocolgame code. The player methods and specially the creatureevents code should be called by the dispatcher thread, you are calling them from the main/asio thread = crash.

I guess this is not very hard to correct. However, I prefer separating this feature into modules, like in OTX.

nekiro · 2020-04-28T12:49:47Z

@cristofermartins interesting, can you tell me in which cases or why would it crash?
cause I was testing and I didnt see any crashes, so I'm curious

nekiro · 2020-04-28T13:37:31Z

@infister to get module "feeling" you can use revscriptsys.

infister · 2020-04-28T14:12:00Z

@cristofermartins interesting, can you tell me in which cases or why would it crash?
cause I was testing and I didnt see any crashes, so I'm curious

try to reload this with syntax error and then receive such packet

@infister to get module "feeling" you can use revscriptsys.

I am not talking about feeling, this just, in my opinion, should be separated from creaturescripts.

SaiyansKing · 2020-04-28T15:20:47Z

@cristofermartins interesting, can you tell me in which cases or why would it crash?
cause I was testing and I didnt see any crashes, so I'm curious

You have a racing condition inside "getParsePacketEvent" if somehow new event will be registered from dispatcher thread and asio thread will be looping through eventsList it'll result in crash.

Also I'm seeing that many people don't understand what is passing object by reference because your function "executeParsePacket" might get different data when it actually get called by dispatcher thread(data can be changed by asio thread).

MillhioreBT · 2020-04-28T15:54:35Z

I prefer to use the OTX method, and it is also compatible with Revscripts ;)

nekiro · 2020-04-28T16:02:19Z

@MillhioreBT "otx" you meant @slavidodo method right? cause their code is stolen from slavi

nekiro · 2020-04-28T16:03:05Z

@SaiyansKing can you enligthen me, how and why would be the data changed by asio thread? I will agree thats something I dont know much so I want to learn, will enjoy a good explanation

MillhioreBT · 2020-04-28T16:38:06Z

@MillhioreBT "otx" you meant @slavidodo method right? cause their code is stolen from slavi

It's pretty good anyway, why switch to yours? is better? I would gladly change if necessary.

nekiro · 2020-04-28T16:40:05Z

@MillhioreBT ofc, you are free to use whatever you like to use. I'm not the one to judge which one is better and if you like their implementation better then use the one that suit you the best.
This is my implementation and that's all.

SaiyansKing · 2020-04-28T16:46:59Z

@SaiyansKing can you enligthen me, how and why would be the data changed by asio thread? I will agree thats something I dont know much so I want to learn, will enjoy a good explanation

Its just that when you had reference to "Connection::msg" asio thread would read new data to that buffer without waiting for your dispatcher task to do its work. It is rather rare racing condition but a possibility is a possibility so it was worth mentioning it.

nekiro · 2020-04-28T16:50:39Z

@SaiyansKing thanks! Can I request your review on that pr? I changed the code a bit I guess it should be fine now?

yamaken93 · 2020-04-28T16:58:24Z

I also have a question(which is related to this pull request): why the packet parsing is not done by the dispatcher thread?

nekiro · 2020-04-28T17:09:06Z

@cristofermartins can you review the pull request, because I'm not sure what you mean now?

SaiyansKing · 2020-04-28T17:33:43Z

@SaiyansKing thanks! Can I request your review on that pr? I changed the code a bit I guess it should be fine now?

~~I'm sorry, I can't do reviews on pr's here~~, it looks ok, however someone might question you why you passed player directly instead of player uid but looking at connection.cpp it shouldn't be possible to queue work before "Protocol::release" will be called so I don't know reason why they used player uid in first place.

I also have a question(which is related to this pull request): why the packet parsing is not done by the dispatcher thread?

If you're asking me then I don't know performance-wise it doesn't matter because at most you read only few bytes and main job still do dispatcher, maybe they didn't want to make unnecessary copies of networkmessage buffer because as I posted before passing it as reference would have rare racing condition.

SaiyansKing

It looks ok to me.

yamaken93 · 2020-04-28T20:40:24Z

Memory leak in the Game::playerExecuteParsePacketEvent, message(pointer to NetworkMessage) is not deleted.

nekiro · 2020-04-28T20:42:11Z

@cristofermartins message is deleted in lua, its the same thing like when you use following code in lua and network message class in lua got garbage collector which calls delete

local msg = NetworkMessage()
msg:delete()

MillhioreBT · 2020-04-29T01:02:17Z

@nekiro Will a situation be possible where it is necessary to keep the pointer alive for an extra time at the end of the event? otherwise, we can clean pointer from fonts to avoid writing an additional line, what do you think?

MillhioreBT

It convinced me, I tested it and it seems to work very well.

nekiro · 2020-04-29T08:28:48Z

@MillhioreBT you shouldnt have to delete the message manually, garbage collector should delete it at the next run, when reference count hits 0 so last msg:delete() isnt necessary, but better safe than sorry.

kygov · 2020-04-29T11:14:01Z

In my opinion we should implement code from otservbr as its much better.
https://github.com/opentibiabr/otservbr-global/blob/develop/src/modules.cpp

nekiro · 2020-04-29T11:15:36Z

@kygov if you want to openly say something is better than something else. You should give us arguments on why.

MillhioreBT

I agree with this, and might even insist on creating multiple instances of this event:
I did it this way and it worked wonderfully:

game.cpp

void Game::playerExecuteParsePacketEvent(uint32_t playerId, uint8_t recvbyte, NetworkMessage* message)
{
	Player* player = getPlayerByID(playerId);
	if (!player) {
		return;
	}

	if (!player->hasEventRegistered(CREATURE_EVENT_PARSE_PACKET)) {
		return;
	}

	for (CreatureEvent* creatureEvent : player->eventsList) {
		if (!creatureEvent->isLoaded()) {
			continue;
		}

		if (creatureEvent->getEventType() == CREATURE_EVENT_PARSE_PACKET && creatureEvent->getRecvbyte() == recvbyte) {
			std::unique_ptr<NetworkMessage> msgPtr(new NetworkMessage(*message));
			creatureEvent->executeParsePacket(player, recvbyte, std::move(msgPtr));
		}
	}

	delete message;
	message = nullptr;
}

Use this Revscript to verify that what I say is true:

data/scripts/parsepacket.lua

local parsePacketEvent = CreatureEvent("ParsePacket")
parsePacketEvent.onParsePacket = function (player, recvbyte, msg)
	-- structure for parseUseItem packet, recbyte: 130 (0x82)
	local pos = msg:getPosition()
	local clientId = msg:getU16()
	local stackpos = msg:getByte()
	local index = msg:getByte()
	print(("Player: %s wants to use item at position (%d, %d, %d) with sprite id %d"):format(player:getName(), pos.x, pos.y, pos.z, clientId))
	msg:delete()
end
parsePacketEvent:recvbyte(130)
parsePacketEvent:register()

local cliportEvent = CreatureEvent("CliportLua")
cliportEvent.onParsePacket = function (player, recvbyte, msg)
	local cliportStatus = player:getStorageValue(131070) == 1
	if not cliportStatus then
		return 
	end
	local pos = msg:getPosition()
	local path = player:getPathTo(pos, 0, 1, false, false, 0)
	if path then
		local fromPos = player:getPosition()
		for i, dir in pairs(path) do
			fromPos:getNextPosition(dir)
		end
		player:teleportTo(fromPos, true)
	end
	msg:delete()
end
cliportEvent:recvbyte(130)
cliportEvent:register()

local loginEvent = CreatureEvent("LoginEventCliport")
loginEvent.onLogin = function (player)
	player:registerEvent("ParsePacket")
	player:registerEvent("CliportLua")
	return true
end
loginEvent:register()

local talkEvent = TalkAction("/cliport")
talkEvent.onSay = function (player, words, param)
	if not player:getGroup():getAccess() then
		return true
	end
	local cliportStatus = player:getStorageValue(131070) == -1
	player:setStorageValue(131070, cliportStatus and 1 or -1)
	if cliportStatus then
		player:sendTextMessage(MESSAGE_EVENT_DEFAULT, "Cliport condition enabled.")
	else
		player:sendTextMessage(MESSAGE_EVENT_DEFAULT, "Cliport condition disabled.")
	end
	return false
end
talkEvent:register()

It seems to work too well without my small changes and also with them.

nekiro · 2020-05-14T23:18:33Z

I was going through the code again and I just got an idea, what about events interface. This pr is currently using creaturescripts, but events looks like a better place for that? What do you think?

infister · 2020-05-15T16:35:16Z

If you could add multiple events for parsing different recvbytes, then it might be okay.

MillhioreBT · 2020-09-30T22:44:01Z

I was going through the code again and I just got an idea, what about events interface. This pr is currently using creaturescripts, but events looks like a better place for that? What do you think?

I like the idea, and now that the PR #2867 is working perfectly, this has a very nice potential.

src/networkmessage.h

Co-authored-by: Damian Jarek <[email protected]>

MillhioreBT · 2021-04-20T07:46:52Z

Hello, I want to revive this PR and suggest that it move to Events, there would be no need to make smart pointers, nor would we need to duplicate the pointer, since with EventCallbacks we can create instances likewise filtering a single event by several lua_calls automatically and this would be cool

nekiro · 2021-04-20T13:01:24Z

Hello, I want to revive this PR and suggest that it move to Events, there would be no need to make smart pointers, nor would we need to duplicate the pointer, since with EventCallbacks we can create instances likewise filtering a single event by several lua_calls automatically and this would be cool

Smart pointers are made to be used.
I don't see any advantage with moving it to events though, besides one disadvantage which is you cannot unregister the execution.

nekiro · 2021-12-05T11:00:25Z

@EvilHero90 remove your stale pending review please

Zbizu · 2021-12-06T20:22:24Z

Wouldn't it be better in events?
What's the point of registering the creatureevent on every single creature when what you really want is just expanding the protocol for everyone on the server?

nekiro · 2021-12-07T10:25:09Z

Wouldn't it be better in events? What's the point of registering the creatureevent on every single creature when what you really want is just expanding the protocol for everyone on the server?

Well probably yes, but also no. It's nice to have control over what you register to players on the other hand events is not a bad idea, though we would have to implement a class to parse these opcodes and store them to know which opcodes are registered in lua, or just store them in container in game class.

Zbizu · 2021-12-07T11:48:07Z

You can just if + return on players you don't want to serve.

MillhioreBT · 2021-12-09T17:05:21Z

Just allow multiple bytes per event and it would be ready to merge, in my opinion.
example:

MillhioreBT · 2021-12-09T17:08:59Z

src/networkmessage.h

@@ -44,6 +44,10 @@ class NetworkMessage
 		enum { MAX_PROTOCOL_BODY_LENGTH = MAX_BODY_LENGTH - 10 };

 		NetworkMessage() = default;
+		NetworkMessage(const NetworkMessage& other) {


This doesn't make sense if you don't really plan to make a copy, I see that you modified PR to not make copies, so why this?

future proof

Although if you ask me, I think that you should allow us to have the freedom of being able to write several events with the same byte, and make a copy of the message for each event that is called, each one decides whether to do more than one event, why put limitations?

then we can do the following:

MillhioreBT · 2021-12-10T18:30:16Z

src/protocolgame.cpp

@@ -526,6 +526,11 @@ void ProtocolGame::parsePacket(NetworkMessage& msg)
 		}
 	}

+	if (player->hasEventRegistered(CREATURE_EVENT_PARSE_PACKET)) {
+		addGameTask(&Game::playerExecuteParsePacketEvent, player->getID(), recvbyte, msg);
+		return;


this is serious? or a joke? XD
the return; <<<

player:registerEvent("parsepacket") >>> the player will stop doing things, the (all)-packet's are simply never parsed in the sources

nekiro added 2 commits April 28, 2020 13:13

parse packet in lua creaturescript

2469505

Revscriptsys support

e9189fe

call the event in dispatcher thread

532d4c2

Suggested changes

aa6055d

nekiro added 2 commits April 28, 2020 19:48

Use game class to execute event

c532f02

Remove extra line

2c28e73

SaiyansKing previously approved these changes Apr 28, 2020

View reviewed changes

MillhioreBT previously approved these changes Apr 29, 2020

View reviewed changes

MillhioreBT previously approved these changes May 4, 2020

View reviewed changes

djarek reviewed Sep 30, 2020

View reviewed changes

src/networkmessage.h Outdated Show resolved Hide resolved

mattyx14 previously approved these changes Oct 1, 2020

View reviewed changes

Missing const in copy operator

3a76f7a

Co-authored-by: Damian Jarek <[email protected]>

nekiro dismissed stale reviews from mattyx14 and MillhioreBT via 3a76f7a October 1, 2020 11:11

ghost mentioned this pull request Nov 26, 2020

[Feature Request] parsePacket to Lua! #1786

Closed

nekiro added 2 commits April 20, 2021 15:28

Don't call the task if player has no event registered

e539514

Fix const name

f9c82a4

MillhioreBT mentioned this pull request Dec 5, 2021

CreatureEvent onParsePacket #3833

Closed

3 tasks

nekiro added 2 commits December 5, 2021 11:33

Merge branch 'master' into parse-packet-lua

907e592

Don't copy network message

1eb89dd

EvilHero90 approved these changes Dec 6, 2021

View reviewed changes

ghost requested review from yamaken93, djarek and EvilHero90 December 6, 2021 18:36

MillhioreBT reviewed Dec 9, 2021

View reviewed changes

MillhioreBT requested changes Dec 10, 2021

View reviewed changes

nekiro closed this Dec 10, 2021

ranisalt mentioned this pull request Mar 29, 2022

Add unknown packet handling in Lua #4043

Merged

3 tasks

Parse packet in lua feature #2978

Parse packet in lua feature #2978

Conversation

nekiro commented Apr 28, 2020 • edited Loading

yamaken93 commented Apr 28, 2020

infister commented Apr 28, 2020

nekiro commented Apr 28, 2020 • edited Loading

nekiro commented Apr 28, 2020

infister commented Apr 28, 2020 • edited Loading

SaiyansKing commented Apr 28, 2020

MillhioreBT commented Apr 28, 2020

nekiro commented Apr 28, 2020

nekiro commented Apr 28, 2020 • edited Loading

MillhioreBT commented Apr 28, 2020 • edited Loading

nekiro commented Apr 28, 2020 • edited Loading

SaiyansKing commented Apr 28, 2020

nekiro commented Apr 28, 2020

yamaken93 commented Apr 28, 2020

nekiro commented Apr 28, 2020 • edited Loading

SaiyansKing commented Apr 28, 2020 • edited Loading

SaiyansKing left a comment

Choose a reason for hiding this comment

yamaken93 commented Apr 28, 2020

nekiro commented Apr 28, 2020 • edited Loading

MillhioreBT commented Apr 29, 2020

MillhioreBT left a comment

Choose a reason for hiding this comment

nekiro commented Apr 29, 2020 • edited Loading

kygov commented Apr 29, 2020

nekiro commented Apr 29, 2020

MillhioreBT left a comment • edited Loading

Choose a reason for hiding this comment

nekiro commented May 14, 2020

infister commented May 15, 2020

MillhioreBT commented Sep 30, 2020

MillhioreBT commented Apr 20, 2021

nekiro commented Apr 20, 2021

nekiro commented Dec 5, 2021

Zbizu commented Dec 6, 2021 • edited Loading

nekiro commented Dec 7, 2021

Zbizu commented Dec 7, 2021

MillhioreBT commented Dec 9, 2021

MillhioreBT Dec 9, 2021

Choose a reason for hiding this comment

nekiro Dec 9, 2021

Choose a reason for hiding this comment

MillhioreBT Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

MillhioreBT Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

nekiro commented Apr 28, 2020 •

edited

Loading

nekiro commented Apr 28, 2020 •

edited

Loading

infister commented Apr 28, 2020 •

edited

Loading

nekiro commented Apr 28, 2020 •

edited

Loading

MillhioreBT commented Apr 28, 2020 •

edited

Loading

nekiro commented Apr 28, 2020 •

edited

Loading

nekiro commented Apr 28, 2020 •

edited

Loading

SaiyansKing commented Apr 28, 2020 •

edited

Loading

nekiro commented Apr 28, 2020 •

edited

Loading

nekiro commented Apr 29, 2020 •

edited

Loading

MillhioreBT left a comment •

edited

Loading

Zbizu commented Dec 6, 2021 •

edited

Loading

MillhioreBT Dec 9, 2021 •

edited

Loading

MillhioreBT Dec 10, 2021 •

edited

Loading