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

L4d2 Plugin Simplification #2650

Merged
merged 3 commits into from Nov 25, 2016

Conversation

@EmperorArthur
Copy link
Contributor

commented Nov 23, 2016

While working on a plugin for a different game I noticed quite a bit of overlap between the Windows and Linux versions of the l4d2 plugin.

I've separated the constants from the program logic, and have combined the two files. This should massively ease future maintenance, and allow for easier updating should those constants change.

Note: I've compiled this on Linux, but can't test it. I do not have a Windows build system at this time.

const procptr32_t serverid_steamclient_offset = 0x95E56D;
const procptr32_t player_server_offset = 0x7F87BC;
const procptr32_t playerid_engine_offset = 0x4EBF88;
//Module Names

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 23, 2016

Member

This comment should be: // Module names

const procptr32_t serverid_steamclient_offset = 0x1216CA5;
const procptr32_t player_server_offset = 0xF340E4;
const procptr32_t playerid_engine_offset = 0xA62C60;
//Module Names

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 23, 2016

Member

This comment should be: // Module names

return false;
}

// Server ID
procptr32_t steamclient = getModuleAddr(L"steamclient.dll"); // Retrieve "steamclient.dll" module's memory address
procptr32_t steamclient = getModuleAddr(steamclient_name); // Retrieve module's memory address

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 23, 2016

Member

This comment should be: // Retrieve "steamclient_name" module's memory address


// Player name
procptr32_t server = getModuleAddr(L"server.dll"); // Retrieve "server.dll" module's memory address
procptr32_t server = getModuleAddr(server_name); // Retrieve module's memory address

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 23, 2016

Member

This comment should be: // Retrieve "server_name" module's memory address


// Player ID
procptr32_t engine = getModuleAddr(L"engine.dll"); // Retrieve "engine.dll" module's memory address
procptr32_t engine = getModuleAddr(engine_name); // Retrieve module's memory address

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 23, 2016

Member

This comment should be: // Retrieve "engine_name" module's memory address

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

Thank you for the PR! 😄
Having a single file would be better when you edit something, however there's a problem: this would break plugins compatibility with Wine.

@EmperorArthur

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2016

Glad to contribute. How does it break wine?

l4d2.pro only had the Windows file on win32, and only had the Linux file on linux. I would love to build all the Windows plugins on Linux, but that currently doesn't happen.

What are your thoughts on using a constants section instead of having them in the code itself? It's what the wow plugin does, and seems a bit more readable than what the wiki currently recommends.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

It doesn't break wine per se. but the process we had in mind for wine plugins would be that Windows plugins have one build dir, and Linux plugins had another one.

For l4d2, this would probably be 'l4d2' for Windows, and 'l4d2_linux' for Linux.

Why separte dirs? Well, qmake can only produce one binary per directory, since a directory can only contain one .pro file. (At least when used with qmake SUBDIRS...)

The idea we had in mind would be that all Windows plugin would be built on Linux, but named something like 'libmumble_paplugin_win32_$TARGET.so'.

So, to be able to accomplish this, we'd need separate dirs for the Windows/wine version vs. the Linux version.

But -- that isn't even the case right now for the l4d2 plugin. Both Windows and Linux currently live in the same directory, so I'm fine with this change if @davidebeatrici thinks it improves readability/maintainability.

We can think about Wine later.
It shouldn't be a big problem to solve: we'd just make a l4d2_linux dir next to the existing l4d2, and simply reference the same l4d2.cpp source file.

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

It shouldn't be a big problem to solve: we'd just make a l4d2_linux dir next to the existing l4d2, and simply reference the same l4d2.cpp source file.

We could keep the folder l4d2 and create two subfolders inside it: win32 and linux.

What are your thoughts on using a constants section instead of having them in the code itself? It's what the wow plugin does, and seems a bit more readable than what the wiki currently recommends.

I agree.
Just a thing: I would prefer to use the modules as in the L4D plugin. I was already planning to do it, before your PR. 😉

static procptr32_t steamclient, server, engine; // Variables to contain modules addresses
(...)
const procptr32_t serverid_offset = 0x1216CA5;
const procptr32_t player_offset = 0xF340E4;
const procptr32_t playerid_offset = 0xA62C60;
(...)
peekProc(steamclient + serverid_offset, serverid) && // Unique server Steam ID.
(...)
peekProc(server + player_offset, player) && // Player nickname.
peekProc(engine + playerid_offset, playerid); // Unique player Steam ID.
(...)
steamclient = getModuleAddr(steamclient_name); // Retrieve "steamclient_name" module's memory address
(...)
server = getModuleAddr(server_name); // Retrieve "server_name" module's memory address
(...)
engine = getModuleAddr(engine_name); // Retrieve "engine_name" module's memory address
(...)
const procptr32_t player_server_offset = 0x7F87BC;
const procptr32_t playerid_engine_offset = 0x4EBF88;
// Module names
const wchar_t * exe_name = L"left4dead2.exe";

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 25, 2016

Member

What about moving the asterisk near the variable type?

const wchar_t* exe_name

This comment has been minimized.

Copy link
@mkrautz

mkrautz Nov 25, 2016

Member

No, that's not our coding style. If anything, it should be

const wchar_t *exe_name

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 25, 2016

Member

Right.
What about the indentation? It seems like there are tabs and spaces mixed.

This comment has been minimized.

Copy link
@EmperorArthur

EmperorArthur Nov 25, 2016

Author Contributor

The joys of switching between projects that use spaces and those that use tabs...

Fixing.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 25, 2016

Member

Are you using Qt Creator? With it you can choose between the default indentation preset and a new one that uses only tabs. 😉

peekProc(pModule + 0x772C28, map) && // Map name.
peekProc(player_server, player) && // Player nickname.
peekProc(playerid_engine, playerid); // Unique player Steam ID.
ok = peekProc(pModule + state_address, &state, 1) && // Magical state value: 0 or 255 when in main menu and 1 when in-game.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Nov 25, 2016

Member

I think that we should use "offset" also for pModule. 😉

peekProc(pModule + state_offset, &state, 1)
@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

I think that the additional tabs are causing problems with the indentation...

@EmperorArthur

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2016

Probably.

My editor's tab to space conversion always brings tabs over to the next col_num %4. So t<tab>, te<tab>, tes<tab> and test all have the same length.

It seems like half the editors do this, and the other half don't.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

Probably makes sense to squash some of the style fixups before merging?

@EmperorArthur EmperorArthur force-pushed the EmperorArthur:l4d2_simplification branch from 9f6c172 to 82acc69 Nov 25, 2016

l4d2 plugin: Moved constants to a single const block
Moved memory addresses and module names to a const block
This affects both the Windows and Linux versions

Minor syntax changes to align both versions of the plugin
l4d2 plugin: combined plugin files
Combined both Windows and Linux plugins into a single file
l4d2 plugin: cleanup
Renamed *_address to *_offset
Aligned whitespace
Moved module calculations to peekProc area

@EmperorArthur EmperorArthur force-pushed the EmperorArthur:l4d2_simplification branch from 82acc69 to 8494361 Nov 25, 2016

@mkrautz

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

OK, looks good enough for me. :-)

@mkrautz mkrautz merged commit db12479 into mumble-voip:master Nov 25, 2016

@EmperorArthur EmperorArthur deleted the EmperorArthur:l4d2_simplification branch Nov 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.