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

Add unknown packet handling in Lua #4043

Merged
merged 8 commits into from
Mar 28, 2023
Merged

Conversation

ranisalt
Copy link
Member

@ranisalt ranisalt commented Mar 25, 2022

Pull Request Prelude

Changes Proposed

This pull request aims to add packet handling in Lua for network messages with unknown packet types. The idea, in the long run, is to move lightweight handlers to Lua for easy extensibility, rather than baking into the source code. New packet types should be added directly with Lua scripts instead.

Closes #1786

@ranisalt ranisalt requested a review from a user March 25, 2022 19:19
src/networkmessage.h Outdated Show resolved Hide resolved
src/protocolgame.cpp Outdated Show resolved Hide resolved
@Zbizu
Copy link
Contributor

Zbizu commented Mar 25, 2022

any advantage for doing this as creaturescript instead of player event?

@ghost ghost added the feature New feature or functionality label Mar 26, 2022
@ranisalt
Copy link
Member Author

any advantage for doing this as creaturescript instead of player event?

None at all! I am just too dumb to know the other alternatives. Appreciate the help 🤣

src/protocolgame.cpp Outdated Show resolved Hide resolved
@Zbizu
Copy link
Contributor

Zbizu commented Mar 26, 2022

src/networkmessage.h Outdated Show resolved Hide resolved
@ranisalt ranisalt force-pushed the lua-packet-handler branch 2 times, most recently from f33031c to 177353b Compare March 26, 2022 19:00
@Zbizu
Copy link
Contributor

Zbizu commented Mar 28, 2022

closes #1786
(if gets merged ofc)

@ranisalt ranisalt marked this pull request as ready for review March 31, 2022 17:29
@ranisalt
Copy link
Member Author

I've been using it for a while now and it's pretty stable. Any suggestions?

@Zbizu
Copy link
Contributor

Zbizu commented Mar 31, 2022

https://github.com/Zbizu/forgottenserver/commit/de2db0c6135d1bf505a158e69fecc68dd65f2077#
if I remember correctly these two packets aren't handled by the code fully, but can be ignored because one situation is scripted in onLogin, the other is handled by noPongTime in vocations.xml so they make pointless calls

src/game.cpp Outdated Show resolved Hide resolved
@nekiro
Copy link
Member

nekiro commented Apr 1, 2022

I've been using it for a while now and it's pretty stable. Any suggestions?

Avoid copying , if we parse only unhandled opcodes then why are we copying?

@ranisalt
Copy link
Member Author

ranisalt commented Apr 1, 2022

if I remember correctly these two packets aren't handled by the code fully, but can be ignored because one situation is scripted in onLogin, the other is handled by noPongTime in vocations.xml so they make pointless calls

Is it a big deal to just if ... then return to skip these two?

Avoid copying , if we parse only unhandled opcodes then why are we copying?

Well, because I don't want the network message to be stored for later usage only to crash the server because it was used after deletion. But maybe it would be better to move the message into parsePacket instead of passing a ref.

@ranisalt ranisalt force-pushed the lua-packet-handler branch from 9155410 to 047e5dc Compare April 3, 2022 00:58
@MillhioreBT
Copy link
Contributor

MillhioreBT commented Apr 10, 2022

When I merge these changes I can't connect with any character, I think the handling of the buffers is not correct
simply trying to enter does nothing, not even the ProtocolGame::parsePacket(NetworkMessage& msg) method is executed correctly

src/events.cpp Outdated Show resolved Hide resolved
@Zbizu
Copy link
Contributor

Zbizu commented Apr 19, 2022

the packets fail to copy (report shows in base64 that entire packet was 0x00)
obraz

I wasn't able to compile any previous commit on that branch

@ranisalt ranisalt mentioned this pull request Jun 4, 2022
3 tasks
@ranisalt ranisalt force-pushed the lua-packet-handler branch 3 times, most recently from 7012ea1 to 807a41e Compare December 13, 2022 20:29
@ranisalt
Copy link
Member Author

With help from @MillhioreBT I believe the issue above has been fixed. The problem was replacing memcpy with copy_n without properly converting data types, I reverted to memcpy which works as expected.

ghost
ghost previously approved these changes Jan 26, 2023
@ghost ghost requested review from DSpeichert, EvilHero90, nekiro, yamaken93 and Zbizu January 26, 2023 23:34
MillhioreBT
MillhioreBT previously approved these changes Jan 27, 2023
Copy link
Contributor

@MillhioreBT MillhioreBT left a comment

Choose a reason for hiding this comment

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

Working correctly.

@ranisalt
Copy link
Member Author

@Zbizu if you have time, please check again that it has been fixed

@ranisalt ranisalt dismissed stale reviews from MillhioreBT and ghost via 014faf8 January 27, 2023 01:19
MillhioreBT
MillhioreBT previously approved these changes Jan 27, 2023
@ranisalt ranisalt requested review from a user and Zbizu and removed request for Zbizu January 27, 2023 09:58
@MillhioreBT MillhioreBT mentioned this pull request Feb 28, 2023
3 tasks
ghost
ghost previously approved these changes Mar 28, 2023
src/events.cpp Outdated
}

if (!scriptInterface.reserveScriptEnv()) {
std::cout << "[Error - Events::eventPlayerOnInventoryUpdate] Call stack overflow" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

eventPlayerOnNetworkMessage

@ranisalt ranisalt dismissed stale reviews from ghost and MillhioreBT via 7485104 March 28, 2023 21:14
@ranisalt ranisalt merged commit 0769e9a into otland:master Mar 28, 2023
@ranisalt ranisalt deleted the lua-packet-handler branch March 29, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] parsePacket to Lua!
5 participants