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.

Sign up for GitHub

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

Jump to bottom

Adding network msg to network msg #4878

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Adding network msg to network msg #4878

wants to merge 5 commits into from

Conversation

ArturKnopik
Copy link
Contributor

@ArturKnopik ArturKnopik commented Dec 19, 2024 •
edited
Loading

Pull Request Prelude

NetworkMessage functionality extension.

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Added ability to append a NetworkMessage to another NetworkMessage, which allows caching of parts that are repeated in the message.

Issues addressed:

suggestion: caching NetworkMessage blocks #4450

How to test: 

local cacheMsg = NetworkMessage()
cacheMsg:addString("test")
cacheMsg:addByte(22)

local msg = NetworkMessage()
msg:addByte(5)
msg:addNetworkMessage(cacheMsg)
msg:addByte(66)

msg:seek(0)
print(msg:getByte())
print(msg:getString())
print(msg:getByte())
print(msg:getByte())

expected output

##### luajit
5
test
22
66
##### lua 5.3
5.0
test
22.0
66.0

@ArturKnopik ArturKnopik added the enhancement Increase or improvement in quality, value, or extent label Dec 19, 2024
ArturKnopik
ArturKnopik commented Dec 19, 2024
View reviewed changes
src/networkmessage.cpp Outdated
@@ -191,3 +191,19 @@ void NetworkMessage::addItem(const Item* item)
}

void NetworkMessage::addItemId(uint16_t itemId) { add<uint16_t>(Item::items[itemId].clientId); }

void NetworkMessage::addNetworkMessage(NetworkMessage* networkMsg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

probaly can be changed from:
void NetworkMessage::addNetworkMessage(NetworkMessage* networkMsg)
to:
void NetworkMessage::addNetworkMessage(NetworkMessage networkMsg)

nekiro
nekiro reviewed Dec 19, 2024
View reviewed changes
src/networkmessage.cpp Outdated
return;
}

std::memcpy(buffer.data() + info.position, networkMsg->getBuffer() + INITIAL_BUFFER_POSITION,
Copy link
Member

Choose a reason for hiding this comment

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

wouldnt addBytes from networkmessage class suffice here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, i will check.

src/luascript.cpp Outdated
@@ -6293,6 +6294,26 @@ int LuaScriptInterface::luaNetworkMessageAddItemId(lua_State* L)
return 1;
}

int LuaScriptInterface::luaNetworkMessageAddNetworkMessage(lua_State* L)
{
// networkMessage:addNetworkMessage(msg)
Copy link
Member

Choose a reason for hiding this comment

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

networkMessage:compose(otherMsg)?

ArturKnopik
ArturKnopik commented Dec 19, 2024
View reviewed changes
src/networkmessage.h
@@ -100,6 +100,7 @@ class NetworkMessage
}

void addBytes(const char* bytes, size_t size);
void addBytes(const uint8_t* bytes, size_t size);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to avoid reinterpret_cast<const char*>

ArturKnopik
ArturKnopik commented Dec 19, 2024
View reviewed changes
src/networkmessage.h
@@ -53,7 +53,14 @@ class NetworkMessage
return buffer[info.position++];
}

uint8_t getPreviousByte() { return buffer[--info.position]; }
// Returns first element of body
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: fix comment

@ArturKnopik ArturKnopik marked this pull request as draft December 22, 2024 07:00
ramon-bernardo
ramon-bernardo reviewed Dec 22, 2024
View reviewed changes
Copy link
Contributor

@ramon-bernardo ramon-bernardo left a comment

Choose a reason for hiding this comment

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

Im not usually a fan of returning a bool just for success or failure, but in this case, there’s a failure when adding the message. Lua won’t actually receive this return, check it out:

src/networkmessage.cpp
Comment on lines +208 to +215
void NetworkMessage::addNetworkMessage(const NetworkMessage& networkMsg)
{
if (!canAdd(networkMsg.getLength())) {
return;
}

addBytes(networkMsg.getBuffer() + INITIAL_BUFFER_POSITION, networkMsg.getLength());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As a suggestion, you can abbreviate networkMsg to msg

src/networkmessage.h
@@ -111,6 +119,7 @@ class NetworkMessage
void addItem(uint16_t id, uint8_t count);
void addItem(const Item* item);
void addItemId(uint16_t itemId);
void addNetworkMessage(const NetworkMessage& networkMsg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion above

src/luascript.cpp
Comment on lines +6312 to +6315
message->addNetworkMessage(*msgToAdd);
tfs::lua::pushBoolean(L, true);
return 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we always return true, even when there’s no space in the msg body

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramon-bernardo ramon-bernardo ramon-bernardo left review comments

@nekiro nekiro nekiro left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement Increase or improvement in quality, value, or extent
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ArturKnopik @nekiro @ramon-bernardo