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

Implement Forge protocol handshake support #3869

Merged
merged 180 commits into from Aug 27, 2017

Conversation

Projects
None yet
6 participants
@satoshinm
Contributor

satoshinm commented Jul 23, 2017

Adds initial support for the Forge handshake protocol, as documented on http://wiki.vg/Minecraft_Forge_Handshake

Includes a Lua API to allow plugins to register as server-side Forge mods, which are included in the server list ping and ModList packet in the handshake exchanged with Forge clients.

No support for adding new game content in this pull request, but this lays the groundwork, and even without it currently allows server plugins to be written to hook the login handshake to require or deny specific client mods (or Forge itself). Example/test plugin: https://github.com/satoshinm/SampleForgeMod.lua/blob/master/main.lua

satoshinm added some commits Jul 9, 2017

Move cForgeHandshake instance to cClientHandle to fix duplication cau…
…sing state discrepancy between cProtocolRecognizer and cProtocol_1_9_0, both cProtocol subclasses
@satoshinm

This comment has been minimized.

Contributor

satoshinm commented Aug 10, 2017

So many versions.. not sure how to best handle changes from unmerged branches, but if #3908 (1.12.1) is merged first, then this PR should be updated to include this change: satoshinm/CuberiteForge@5c95c2a. I merged the two branches together and tested with this plugin change: satoshinm/SampleForgeMod.lua@e6a1ea8 and it showed up correctly on 1.12.1:

screen shot 2017-08-09 at 8 48 22 pm

@satoshinm

This comment has been minimized.

Contributor

satoshinm commented Aug 12, 2017

Any other changes needed?

Message.push_back('\0');
Message.push_back('\0');
Message.push_back('\0');
Message.push_back('\0');

This comment has been minimized.

@lkolbly

lkolbly Aug 12, 2017

Contributor

Here, would it make sense to use a cByteBuffer? I haven't tried it, but I think you could do:

cByteBuffer Buf(6);
Buf.WriteBEInt8(Discriminator::ServerHello);
Buf.WriteBEInt8(2);
Buf.WriteBEInt32(0); // I assume...?
Buf.ReadAll(Message);

And then I think the types of the fields would be more clear.

/** Server handshake state phases. */
namespace ServerPhase
{
static const Int8 WAITINGCACK = 2;

This comment has been minimized.

@lkolbly

lkolbly Aug 12, 2017

Contributor

WAITING_CLIENT_ACK?

This comment has been minimized.

@lkolbly

lkolbly Aug 12, 2017

Contributor

WaitingClientAck if you want to match the Discriminator camelcasing.

This comment has been minimized.

@satoshinm

satoshinm Aug 26, 2017

Contributor

WAITINGCACK matches what Forge calls it, not essential for interoperability but I think it's worthwhile to match their conventions to reduce confusion

AString ClientModsString;
for (auto & item: ClientMods)
{
ClientModsString.append(item.first);

This comment has been minimized.

@lkolbly

lkolbly Aug 12, 2017

Contributor

Would

ClientModsString.append(Printf("%s@%s, ", item.first, item.second))

work? It might be slightly slower, but it'd be one line instead of four.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 17, 2017

Member

We even have an AppendPrintf function :)

This comment has been minimized.

@lkolbly

lkolbly Aug 19, 2017

Contributor

Ooh, nifty. That's planning ahead.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 21, 2017

Member

Actually, it's just an implementation detail of how all the Printf-like functions are implemented - they all go through this one :)

This comment has been minimized.

@satoshinm

satoshinm Aug 26, 2017

Contributor

Slight change to pass C strings, cleaner and works well:

AppendPrintf(ClientModsString, "%s@%s, ", item.first.c_str(), item.second.c_str());
@madmaxoft

This comment has been minimized.

Member

madmaxoft commented Aug 17, 2017

Okay, I agree with merge. @lkolbly 's notes would be good to address, but I don't think they're absolutely critical. :)

@madmaxoft

This comment has been minimized.

Member

madmaxoft commented Aug 17, 2017

I've merged the 1.12.1 support, so now this can be updated.

@madmaxoft

This comment has been minimized.

Member

madmaxoft commented Aug 25, 2017

The builds are still failing :(

@satoshinm

This comment has been minimized.

Contributor

satoshinm commented Aug 26, 2017

Merge errors, will investigate

@satoshinm satoshinm changed the title from Implement Forge protocol handshake support to [WIP] Implement Forge protocol handshake support Aug 26, 2017

@satoshinm satoshinm changed the title from [WIP] Implement Forge protocol handshake support to Implement Forge protocol handshake support Aug 26, 2017

@satoshinm

This comment has been minimized.

Contributor

satoshinm commented Aug 26, 2017

Fixed, I think this is ready finally...




void cServer::UnregisterForgeMod(AString &a_ModName, UInt32 a_ProtocolVersionNumber)

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 26, 2017

Contributor

Please change to const, tolua returns params that are AString references. Marking it as const avoids it:
AString &a_ModName -> const AString & a_ModName

Example:
cRoot:Get():GetServer():UnregisterForgeMod('JustAString', 1)
returns
JustAString

This comment has been minimized.

@madmaxoft

madmaxoft Aug 26, 2017

Member

Wow, I've missed that one (and the style issue). Good thing I'm not the only reviewer :)

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 26, 2017

Contributor

Sorry I cheated, APIFuzzing...

This comment has been minimized.

@madmaxoft

madmaxoft Aug 26, 2017

Member

Anything that discovers problems is good, even better if it's automatic :)
Might be reasonable to add APIFuzzing to the CI builds.

This comment has been minimized.

@Seadragon91

Seadragon91 Aug 26, 2017

Contributor

@madmaxoft Created a forum post

This comment has been minimized.

@satoshinm

satoshinm Aug 26, 2017

Contributor

Added const to the AString references in both RegisterForgeMod and UnregisterForgeMod

@@ -202,6 +215,9 @@ class cServer
size_t m_MaxPlayers;
bool m_bIsHardcore;

/** Map of protocol version to Forge mods for that version. */
std::map<UInt32, AStringMap> m_ForgeModsByVersion;

This comment has been minimized.

@madmaxoft

madmaxoft Aug 26, 2017

Member

Since you're already touching this file due to @Seadragon91 's review, might be worth detailing what this is:

/** Map of protocol version to Forge mods (map of ModName -> ModVersionString) for that version. */

Perhaps add it to the public getter, too.

@madmaxoft madmaxoft merged commit 6bc5031 into cuberite:master Aug 27, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment