neverlosecc / source2gen

Source2 games SDK generator
http://s2gen.com
Apache License 2.0
193 stars 31 forks source link

Source2Gen is an executable + Linux support #47

Open Cre3per opened 2 months ago

Cre3per commented 2 months ago

See issue #48 for details and status

Thanks for .clang-format, cmake, and for using assertions! It made working on this project much easier than some other projects.

Cre3per commented 2 months ago

Oh wow, good job. I took a quick look at the changes, and here are some notes. Not really a big fan of macros since we are coding in C++ and not C. We were trying to remove our macros a while ago, and now we are getting some new ones :) This definitely needs lots of refactoring, but good job nonetheless

Thanks for the quick review and your attention to C++! I'm with you on avoiding macros as much as we can.
We'll use a sort of offset class to avoid uses of IF_LINUX/WINDOWS. There'll be some macros for pads that exist on a single platform, unless we use [[no_unique_address]] and zero-sized pads, ever tried that? EDIT: eww, no to [[no_unique_address]]

I had to un-constexpr the requiredModules array in startup.cpp because I didn't want to use LOADER_GET_MODULE_FILE_NAME(). Do you have an idea if/how we can build module file names at compile-time without macros?

es3n1n commented 2 months ago

There'll be some macros for pads that exist on a single platform, unless we use [[no_unique_address]] and zero-sized pads, ever tried that? EDIT: eww, no to [[no_unique_address]]

Never even heard of it, lol

I had to un-constexpr the requiredModules array in startup.cpp because I didn't want to use LOADER_GET_MODULE_FILE_NAME(). Do you have an idea if/how we can build module file names at compile-time without macros?

There should definitely be a way to do that. I will try to come up with an implementation, but I am a bit busy with some other projects, so this probably could take some time

cpz commented 2 months ago

Also,

As I know, all these functions where the second parameter is the return value only apply to windows, so you probably need to modify these functions to work on unix

image

Edit: Just saw your comment in code

image

Isnt why we have different kSchemaSystemVersion. We need different schema system version because of support for older games than cs2 or dota2 (e.g. Dota Underlords)

cpz commented 2 months ago

Providing a path for the dumper to the game is convenient if you only use the dumper on the server, but if you use it yourself, you'll get a little tired of it after a while. So on windows we'll most likely have to use Registry and from there take SteamLibrary path and hardcode the folder name for each game. Which sounds a little bit bad, but I don't know any other way. If path was provided then we'll work with it otherwise using registry + hardcoded names of folders for each game.

es3n1n commented 2 months ago

Providing a path for the dumper to the game is convenient if you only use the dumper on the server, but if you use it yourself, you'll get a little tired of it after a while. So on windows we'll most likely have to use Registry and from there take SteamLibrary path and hardcode the folder name for each game. Which sounds a little bit bad, but I don't know any other way. If path was provided then we'll work with it otherwise using registry + hardcoded names of folders for each game.

What's wrong with just placing generated SDKs in the same folder as the binary itself tho? We'll provide an optional path where to place the generated SDKs, and if it wasn't provided, we'll just drop it to the current directory

cpz commented 2 months ago

Providing a path for the dumper to the game is convenient if you only use the dumper on the server, but if you use it yourself, you'll get a little tired of it after a while. So on windows we'll most likely have to use Registry and from there take SteamLibrary path and hardcode the folder name for each game. Which sounds a little bit bad, but I don't know any other way. If path was provided then we'll work with it otherwise using registry + hardcoded names of folders for each game.

What's wrong with just placing generated SDKs in the same folder as the binary itself tho? We'll provide an optional path where to place the generated SDKs, and if it wasn't provided, we'll just drop it to the current directory

We are talkin about game DLL's. Path is being used to get them and load them in to memory. I'm aint against dumping in current executable folder sdk, I thinkin that if I will pass path to dota 2 each time when I want to dump -- I'll break my monitor in week.

Cre3per commented 2 months ago

Also,

As I know, all these functions where the second parameter is the return value only apply to windows, so you probably need to modify these functions to work on unix image

Edit: Just saw your comment in code image

Isnt why we have different kSchemaSystemVersion. We need different schema system version because of support for older games than cs2 or dota2 (e.g. Dota Underlords)

Without knowing the history of source 2, I think "support older games" and "return value optimization" aren't conflicting each other. Could it be that older games or different platforms simply didn't use return value optimization? If that's the case, what we're seeing with these functions might not be caused by versioning, but changed compiler settings on Valve's side.
If this were caused by a change in APIs, I think it'd be weird that it differs between platforms, as Valve is very likely compiling all platforms from the same source tree.
But again, I have no experience with source 2. If you think there are different versions of this function, I'll update the comment.

Cre3per commented 2 months ago

Providing a path for the dumper to the game is convenient if you only use the dumper on the server, but if you use it yourself, you'll get a little tired of it after a while. So on windows we'll most likely have to use Registry and from there take SteamLibrary path and hardcode the folder name for each game. Which sounds a little bit bad, but I don't know any other way. If path was provided then we'll work with it otherwise using registry + hardcoded names of folders for each game.

I've had thoughts in a similar direction. PATH or LD_LIBRARY_PATH needs to contain
cs2/game/bin/linuxsteamrt64/ and
cs2/game/csgo/bin/linuxsteamrt64/

Source2Gen could generate the two paths itself, given a path to the game installation directory. I've added a script (scripts/run.sh)* that does this on Linux. We could add a similar script for Windows.
Linux users, in particular developers, who are the users of source2gen, will know where their game is installed and they'll know how to use their shell's history. An automatic lookup of the game directory would be of little benefit for them.

You have suggested to use the registry on Windows, should we note that idea in the linked issue so whoever restores Windows compatibility has an easier time implementing it?

* source2gen can't set PATH or LD_PRELOAD_PATH itself because the dynamic linker reads those variables once at program launch. Changing them in the program has no effect. That's how it works on Linux at least.

es3n1n commented 2 months ago

Sorry for the delays in my response(and sorry for my previous response; I was sleep-deprived, T_T). I was a bit busy with some other projects, but now I am more or less free from them, so you can expect faster replies from me.

You have suggested to use the registry on Windows, should we note that idea in the linked issue so whoever restores Windows compatibility has an easier time implementing it?

Yes, please. Most likely this person would be me anyway.

Without knowing the history of source 2, I think "support older games" and "return value optimization" aren't conflicting each other. Could it be that older games or different platforms simply didn't use return value optimization? If that's the case, what we're seeing with these functions might not be caused by versioning, but changed compiler settings on Valve's side. If this were caused by a change in APIs, I think it'd be weird that it differs between platforms, as Valve is very likely compiling all platforms from the same source tree. But again, I have no experience with source 2. If you think there are different versions of this function, I'll update the comment.

Yes, indeed. they might've changed compiler options, but since we are no valve employees we cannot know this for sure, this is why this is called a version, this variable would be used in the future if source 2 developers introduce any more changes to the compiler options/functions/anything else. This is just more convenient.

I am OK with having this comment; just add there at least for now and we're good :)

What are the current merge blockers for this PR? Apart from the windows support.

Cre3per commented 2 months ago

Sorry for the delays in my response(and sorry for my previous response; I was sleep-deprived, T_T). I was a bit busy with some other projects, but now I am more or less free from them, so you can expect faster replies from me.

You have suggested to use the registry on Windows, should we note that idea in the linked issue so whoever restores Windows compatibility has an easier time implementing it?

Yes, please. Most likely this person would be me anyway.

Without knowing the history of source 2, I think "support older games" and "return value optimization" aren't conflicting each other. Could it be that older games or different platforms simply didn't use return value optimization? If that's the case, what we're seeing with these functions might not be caused by versioning, but changed compiler settings on Valve's side. If this were caused by a change in APIs, I think it'd be weird that it differs between platforms, as Valve is very likely compiling all platforms from the same source tree. But again, I have no experience with source 2. If you think there are different versions of this function, I'll update the comment.

Yes, indeed. they might've changed compiler options, but since we are no valve employees we cannot know this for sure, this is why this is called a version, this variable would be used in the future if source 2 developers introduce any more changes to the compiler options/functions/anything else. This is just more convenient.

I am OK with having this comment; just add there at least for now and we're good :)

What are the current merge blockers for this PR? Apart from the windows support.

It might be worth to consider adding automatic tests (Separate issue) now that source2gen is faster and doesn't need the games to be running. Even if the tests can only run on a developer machine, they'll make it safer to work on source2gen and could reduce the effort required to review pull requests.

es3n1n commented 2 months ago
* @cpz's [comments](https://github.com/neverlosecc/source2gen/pull/47#pullrequestreview-2015741031). Waiting for a re-review.

Okie, I will let him know.

* Use of macros and missing `constexpr`. This is done from my side. We've had some good ideas in previous threads. If you've got more ideas, I'd be happy to improve the code.

Yeah, I will try to check it out today.

  * For CS2, source2gen generates an SDK that _looks_ right. For now, I can't run CS2 and don't have a reference SDK. I'd come back with fixes once I am a user of the SDK.

Sure I guess, users will complain if something won't work anyway

  * I haven't tested any games apart from CS2, source2gen might well crash for other games. Assuming the person who re-adds Windows support already has all the other games downloaded, I suggest they test those on Windows only. It's very possible that the `get_required_modules()` array differs between games. We could keep other Linux games "potentially broken" until CS2 is confirmed working.

Yeah, we will test it only on Windows. As for the required modules, we previously had an array that worked for all the source 2 games. I am too lazy to check, but if you didn't change the checked modules themselves, then we should probably be good.

It might be worth to consider adding automatic tests (Separate issue) now that source2gen is faster and doesn't need the games to be running. Even if the tests can only run on a developer machine, they'll make it safer to work on source2gen and could reduce the effort required to review pull requests.

Ah yes, I've been dreaming about it for a long time, adding something like GTest for tests would make the further work much easier. But that is a different story and will be implemented later.

-- Once cpz approves all the changes, I guess I will start on adding Windows support back and will most likely refactor other parts as well. This would probably need your help at some point, but we are too far from that as of now.

Cre3per commented 2 months ago
  * I haven't tested any games apart from CS2, source2gen might well crash for other games. Assuming the person who re-adds Windows support already has all the other games downloaded, I suggest they test those on Windows only. It's very possible that the `get_required_modules()` array differs between games. We could keep other Linux games "potentially broken" until CS2 is confirmed working.

Yeah, we will test it only on Windows. As for the required modules, we previously had an array that worked for all the source 2 games. I am too lazy to check, but if you didn't change the checked modules themselves, then we should probably be good.

The target game doesn't need to run anymore for source2gen to work. If the game doesn't load its libraries, someone else has to. That someone is source2gen. The get_required_modules() array now lists all libraries that register a schema.
For CS2, I looked at the example SDK linked on main to figure out what libraries can be dumped and listed those. If the set of libraries differs between games, we could maintain a list per-game, or load all libraries and ignore those that don't expose a InstallSchemaBindings() symbol.

cpz commented 2 months ago

I've did some changes for compiling on Windows and dumping, there was problems with static_asserts and find_module_symbol.

find_module_symbol in loader can be changed, so it wont be based on defines, I've made like this because didnt have enough time to think, sadly atm I have limited time to do anything.

Otherwise, seems like it works. We should somehow make it work based on registry on Windows cos atm for testing I was just placing needed dlls near source2gen executable.

image