WIP: Remove hardcoded Unvanquished references by DefaultUser · Pull Request #16 · DaemonEngine/Daemon

DefaultUser · 2017-04-12T15:48:03Z

This adds support for other games by unhardcoding several references to
Unvanquished.
This switch automatically changes:

default home path
displayed product and version info
default basepak
masterserver, URI scheme and www_baseurl
IRC channel and server

In my opinion, PRODUCT_VERSION/getGameinfo()->version should be defined by the game's Menu code, but for now I will leave it in.

mbasaglia · 2017-04-12T17:13:51Z

Going to link this here since this seems related: Unvanquished/Unvanquished#817

mbasaglia · 2017-04-12T17:17:04Z

src/common/Defs.h

+#undef game
+#define game(x) { PRODUCT_NAME_##x, PRODUCT_NAME_LOWER_##x, PRODUCT_VERSION_##x, DEFAULT_BASE_PAK_##x, GAMENAME_STRING_##x, GAMENAME_FOR_MASTER_##x, MASTER_SERVER_NAME_##x, IRC_SERVER_##x, IRC_CHANNEL_##x, WWW_BASEURL_##x, UNNAMED_SERVER_##x }
+
+const gameinfo_t gamesinfo [ ] = { GAMES };


I really don't like the macro-based approach and the overall C-ness of gameinfo_t, I think it could lead to nasty issues and make it harder to extend in the future

mbasaglia · 2017-04-12T17:18:47Z

src/common/System.h

 #ifndef COMMON_SYSTEM_H_
 #define COMMON_SYSTEM_H_

+extern const gameinfo_t* current_game;


Why do you use current_game directly if you have a getter? I think this should be an implementation detal.

mbasaglia · 2017-04-12T17:19:49Z

src/common/Defs.h


-#define IRC_SERVER          "irc.freenode.org"
-#define IRC_CHANNEL         "unv-lobby"
+#define WWW_BASEURL_UNV         "dl.unvanquished.net/pkg"


Some of these could even become cvars.

cvars for this kind of stuff sounds good

I feel an UNV-suffixed definition or Cvar is still a branding that shouldn't be present in a game-agnostic engine.

The cvar doesn't need the unv suffix

src/engine/client/cl_irc.cpp

mbasaglia · 2017-04-12T17:23:22Z

src/engine/qcommon/files.cpp


 // Cvars to select the base and extra packages to use
-static Cvar::Cvar<std::string> fs_basepak("fs_basepak", "base pak to load", 0, DEFAULT_BASE_PAK);
+static Cvar::Cvar<std::string>* fs_basepak;


Why does this need to be a pointer? Can't you just change its value instead of allocating a new one?

Looks like this is also what breaks the build.

src/engine/qcommon/qcommon.h

mbasaglia · 2017-04-12T17:26:52Z

src/engine/server/sv_main.cpp

-#define HEARTBEAT_GAME PRODUCT_NAME
-#define HEARTBEAT_DEAD PRODUCT_NAME "-dead"
+#define HEARTBEAT_GAME getGameinfo()->name
+#define HEARTBEAT_DEAD va("%s-dead", HEARTBEAT_GAME)


I think we can do without macro declarations for these.

DolceTriade · 2017-04-12T18:03:30Z

I don't think we need to have this be a runtime thing. I don't think using the same game engine binary for different games should really be a goal. Each game should set overriding defines and have these compiled into their binary. I think Unvanquished made a mistake when we released our game with a "daemon" binary instead of an "unvanquished" binary.

That said, I do agree we need to un-hardcode Unv references everywhere.

mbasaglia · 2017-04-12T18:52:10Z

I think we should ensure the client engine is able to work regardless of the game it came from.

illwieckz · 2017-04-12T20:47:59Z

That's not a problem to just rename daemon to unvanquished for convenience at build time, even if the engine is generic enough to run another game.

Kangz · 2017-04-12T21:16:48Z

It is a worthy goal to have a single Daemon binary be able to run all games seamlessly and not have any hardcoded references to Unv. In most cases replacing "Unvanquished" with "Daemon" should be enough.

However I don't think having a single "daemon.exe" for all games will be convenient in the short-term. On example is that all of Unv's packages are versioned and that we'll want to force the usage of .dpk over .pk3 (see #14). On the contrary Xonotic will want support for legacy packages to stay compatible with their maps and mods. I don't think it makes sense to be able to switch between the two modes at runtime as it would lower security if somehow servers can expose game mods that dynamically switch on support for legacy packages.

mbasaglia · 2017-04-12T21:20:03Z

That can be enforced by cvars on the client side.

illwieckz · 2017-04-12T21:38:57Z

not related to that current topic but:

On example is that all of Unv's packages are versioned and that we'll want to force the usage of .dpk over .pk3 (see #14). On the contrary Xonotic will want support for legacy packages to stay compatible with their maps

All the work done in #14 is thought to make Xonotic being able to use dpk for official assets one day (with version, deps, checksum and all awesome features), loading legacy contrib map as compat ones. I don't think it makes sense for a game using Dæmon engine to rely on broken pk3 vfs for their own official assets they can repackage, insted of getting the best from the engine.

To give another example with a slightly different use case when games with open source engine and legacy official closed source assets from company don't allowing modifictions, like wolf:et. These games can't repackage the pak*.pk3, but they can put these pak0.pk3, pak1.pk3 as DEPS in wolfet_4.3.dpk (containing the nacl game code setting fs_legacypaks to true). I think it's not wrong if dpk is the default assumed format, it would be silly to package in legacy pk3 container a game code you have right to develop and package, by the way the code in #14 does not prevent using pk3 for the game pak is fs_legacypaks is set first, but the best way to set it is to set it in game code if you don't want to rely on some scripts. Since Dæmon does not load legacy paks by default, the best way to provide seamless user experience is to put a game code in a dpk that sets fs_legacypaks to true then insta reload vfs to load missing stuff from legacy pk3. if not fs_legacypaks; set fs_legacy paks on; vfs_restart or something like that.

DefaultUser · 2017-04-18T13:15:15Z

I removed the "-game" switch again, instead the gameinfo will be read from a file called "gameinfo.cfg" located in the libpath at runtime.

This does break compiling the unvanquished game logic since the new "Gameinfo" class depends on file access for "libpath". This can be fixed by redefining "Q3_VERSION" in the game logic.

DolceTriade · 2017-04-18T18:20:44Z

Sorry if I'm being a pain, but I'm not sure this is any better than having the information compiled into the binary. The libpath dependency makes it so that you cannot use the same binary with different games without changing gameino.cfg. It doesn't seem there is a way to change this dynamically, and even if there were, i'm not sure there is a strong benefit to this.

Still strongly in favor of simply compiling all of this information into the binary to decrease complexity.

DefaultUser · 2017-04-18T18:25:27Z

Actually you can use the same binary for different games as "libpath" can be set in the commandline arguments.

Kangz · 2017-04-18T19:08:51Z

I'm still not happy with the design of this and agree with @DolceTriade. There are also a whole lot of coding style / nits I want to make but will wait for the final design before doing so to avoid churn. Please refrain from merging before that "style" review.

illwieckz · 2017-04-30T22:58:41Z

I don't see the point of having a named engine or having the engine knowing how to launch the game. Isn't it the launcher's job? The Quake Live engine is named quakezero internally, the Natural Selection 2 engine is named spark, no one cares. The engine is just another component of the game.

Having the engine named like the game and having the engine embedding hardcoded game reference is only needed when then engine is also the launcher. We already have a launcher. If one day Unvanquished is distributed by some game directory like Steam, users will not care about the engine name and people will not need things to be hardcoded.

To host a GoldenEye:Source server I just do: srcds_i486 -game ../gesource, using an unmodified source engine, the -game option just need a path, like our -libpath can do. @DefaultUser 's suggest make Dæmon having the same behavior, which is very common and expected by people hosting games. People playing games don't care, they use the launcher after all.

DefaultUser · 2017-05-04T12:41:32Z

src/engine/qcommon/qcommon.h

 #define PROTOCOL_VERSION       86

-#define URI_SCHEME             GAMENAME_STRING "://"
+#define URI_SCHEME             "dgp://"


Looking at this I am no longer sure that the URI_SCHEME can be engine specific. How should the OS know what game to start when you click a link with an engine specific URI_SCHEME?

External tools would only need to call the engine, the game for the server should be part of the out of band protocol, I think getstatus shows it.

@mbasaglia with current file layout it works only if you mix assets from different games within same directories, which is ok for small mods but not for complete different games (I don't want to share the same pkg/ dir for both xonotic and unvanquished games for example) so the engine must known the game before joining the server 😕 or the file layout must be completely redesigned.

By the way, it's not engine's task to redirect url calls to engine, not a problem to have both unvanquished and xonotic reads the url scheme if the desktop knows how to redirect these url to xonotic or unvanquished (I don't have magical solution, though). The problem of this kind of url stuff is it's more the job for a launcher (like the unvanquished updater), or a job for a game library manager like Steam or Lutris. If we want a common url for multiple games, it's something to design outside of Unvanquished : it's designing a standard with protocols, formats and common tools others can reuse, praising for adoption. 😐

We can keep unv:// for the moment with a glorious FIXME: comment, the protocol is by definition something about others, so we have to take time to think properly about it, and it must not be a blocker for other things.

@DefaultUser as it seems too early to think about protocol uri scheme and I don't want this to block the PR, can you revert this line to unv:// and add a FIXME: comment?

After deep thoughts (I'm not sure) and long time passed (for sure), it now feels stupid to me to have the URI_SCHEME to be engine specific. What we need in engine is a way to grok the URI_SCHEME the game developer wants it to grok. It's up to the game developer to properly configure player system to redirect given URI_SCHEME to the engine with proper options (libpath containing the config file telling the URI_SCHEME proto).

So it now looks very OK to me to set that value in the config file.

Maybe name that constant URI_PROTO to be more explicit than GAMENAME_STRING.
Years later I completely forgotten what GAMENAME_STRING meant and I had to read the code to understand it was the URI proto string.

illwieckz · 2017-07-10T09:33:30Z

In my opinion, PRODUCT_VERSION/getGameinfo()->version should be defined by the game's Menu code, but for now I will leave it in.

I guess there must be two versions: the engine version (hardcoded), and the game version (defined by game code).

origintopleft · 2017-10-11T16:05:06Z

If we were going to insist on daemon somehow being able to figure out what game you meant, we could combine it with the idea of naming the binary after the game - if -libpath is not set at launch, try the binary's name first, then try pkg.

e.g. an unvanquished binary with no -libpath option would first try the unvanquished path, then fall back to pkg if that doesn't work.

illwieckz · 2017-10-11T16:25:25Z

wat?

MarioSMB · 2018-05-13T16:41:22Z

Any news on this @illwieckz @Kangz?

slipher · 2018-05-14T02:31:59Z

Come over to #108 for more in-depth discussion of these issues.

illwieckz · 2018-10-21T16:21:55Z

To me having a simple config files with that:

default home path
displayed product and version info
default basepak
masterservers, and www_baseurl

can be done the proposed way and I don't see any reason to not do it or to wait for something else.

URI scheme can be discussed later, and well, it has to be discussed later. That problem is more complex because it's not how the engine initialize itself but how third-party tools talk to the engine a game specific way. It's not a big problem to fix other issues and we keep unv:// hardcoded until we find a better solution. We can also make the -connect option looking for [a-z]*:// but that would be a very bad idea: the protocol is unique despite being usable by multiple game, hijacking the protocol name to pass a variable is very ugly. Doing something like <protocol>://<game-name>/<server-address:port[/reserved-for-future-usage] would be really more cleaner, it would allow urls like dgp://unvanquished/gg.illwieckz.net:27960 and it would rely on an internal string defined in gameinfo.cfg and it would be enough to prevent a game to mistakenly connect to a server for a different game than the one loaded from libpath. But well, this is another topic.

IRC channel and server thing can be discussed later, and I'm sorry but it is now made entirely obsolete with the need to mute unregistered clients due to increasing IRC spam, and it's obviously not possible to store an irc nick password in a config file distributed to anyone. The problem and its solution is out of topic.

@DolceTriade: The libpath dependency makes it so that you cannot use the same binary with different games without changing gameinfo.cfg.

The purpose of gameinfo.cfg is to be stored in libpath and distributed with vm. It's like loading a shared lib or a nexe except it's not compiled but interpreted since it's a config file. If the current code is not able to read the gameinfo.cfg file before doing something that is required to read it or another chicken-and-egg problem, it means those other things have to be thought another way because there is probably no more convenient way to customize the engine than reading a config file from libpath . And it looks safe to me, another weirdo technical solution would be to have the gamecode loaded from libpath telling the engine what to do but it's obviously not something to do, not only because how uneasy it would be to implement it, but for obvious security reason: randomly autodownloaded gamecode must not be able to tell the engine to read and write a random dir in user's filesystem. That's why the config file in libpath looks really great.

This also means we can go the “very small config file” for this now and talk later for other issues. Unlike the file-system initialisation, there is no security issue in having the game code tweaking the surfaceflags in runtime, for example.

illwieckz · 2019-12-01T00:01:10Z

@DefaultUser, can you rebase this on master? I would like to experiment with it. :-)

gameinfo.cfg needs to be located in libpath

…ranch

DefaultUser · 2019-12-01T15:39:39Z

I cherry-picked the relevant commits and force-pushed the result

src/engine/qcommon/gameinfo.h

In scope of this engine .cfg files are filled with commands. The Gameinfo file does not contain commands

illwieckz · 2019-12-27T21:20:04Z

gameinfo.conf.template

@@ -0,0 +1,7 @@
+NAME               GameName
+VERSION            0.0
+DEFAULT_BASEPAK    mybasepak


why not just BASEPAK? We already know this is the default since it's stored in that file.

After deep thoughts, this may be called BASENAME, there may be more than one usage for a basename.

We can also use it to compute ~/.local/share/${BASENAME} while other OS may do something with NAME instead if it's more common on such OS to do that (My Games/${GAME}).

I don't think it's a bad idea to enforce the basename is also the basepak name.

It may also make easier to reimplement per-directory full-conversion downloadable mod like quake3 did, by using this BASENAME (unvanquished or quake6) string instead of a shortcut (unv or baseq6).

illwieckz · 2019-12-28T16:55:53Z

gameinfo.conf.template

+MASTERSERVER1      master.example.net
+MASTERSERVER2      master.template.net
+WWW_BASEURL        dl.example.net/pkg
+GAMENAME_STRING    abc


URI_PROTO seems to be better or something like that.

illwieckz · 2019-12-28T17:05:03Z

gameinfo.conf.template

@@ -0,0 +1,7 @@
+NAME               GameName
+VERSION            0.0
+DEFAULT_BASEPAK    mybasepak


After deep thoughts, this may be called BASENAME, there may be more than one usage for a basename.

We can also use it to compute ~/.local/share/${BASENAME} while other OS may do something with NAME instead if it's more common on such OS to do that (My Games/${GAME}).

I don't think it's a bad idea to enforce the basename is also the basepak name.

It may also make easier to reimplement per-directory full-conversion downloadable mod like quake3 did, by using this BASENAME (unvanquished or quake6) string instead of a shortcut (unv or baseq6).

illwieckz · 2019-12-28T17:08:20Z

gameinfo.conf.template

@@ -0,0 +1,7 @@
+NAME               GameName


I don't think anything prevents to use a real-life example like Unvanquished default strings in the template.

Same for the default values in the code if undefined.

Note that in the future we would be able to make the engine being able to load some built-in asset and display an error (like Darkplaces does) so the template and the hardcoded version would reference that dummy game that does nothing at all instead.

illwieckz · 2020-01-19T17:30:09Z

This is an idea I just got, let's imagine those two .cfg file (yes, .cfg):

builtin in_name "Unvanquished"
builtin in_baseName "unvanquished"
builtin in_basePak "unvanquished"
builtin in_version "0.51.2"
builtin in_master1 "master.unvanquished.net"
builtin in_master2 "master2.unvanquished.net"
builtin in_wwwBaseURL "dl.unvanquished.net/pkg"
builtin in_protoUrl "unv"

builtin in_name "Xonotuc"
builtin in_baseName "xonotic"
builtin in_basePak "xonotic"
builtin in_version "0.9.0"
builtin in_master1 "master.xonotic.org"
builtin in_master2 "master2.xonotic.org"
builtin in_wwwBaseURL "dl.xonotic.org/pkg"
builtin in_protoUrl "xon"
set fs_legacyPaks on
set fs_maxSymlinkDepth 1
set r_dpMaterial on
set r_dpBlend on

Your non-cfg parser would read all lines starting with ^builtin and only them, this is only written to mimic the .cfg file syntax for cosmetic purpose.

Then, we would hack the command line parser to read every line that does not start with a builtin or a //, and pass them to the command line interpreter as -line, before reading the true command line options, so they can be overridden.

Note that some of these builtins may use normal cvars, like the sv_basePak one. But by doing a in_basePak builtin we configure the one to use as fallback when the one set in cvar is not found.
We may even just not implement the ability to have a default basepak that is not the basename, and use the basename for the basepak in all case. I just kept that in_basePak for the example of a builtin that is not a cvar, even if a cvar exists.

I imagined that because Xonotic would win a lot from being able to set compatibility cvars in that file, about those, an user may choose to disable them without breaking the game itself, but they must be set by default.

illwieckz · 2020-01-19T17:33:51Z

Also, I don't think it's a good idea to set the version in that file. Either the engine has a version, whatever the game, and it's better to hardcode it, either the game has a version, end the game can set it without recompiling the engine.

So, basically, I think that is good:

builtin in_name "Unvanquished"
builtin in_baseName "unvanquished"
builtin in_master1 "master.unvanquished.net"
builtin in_master2 "master2.unvanquished.net"
builtin in_wwwBaseURL "dl.unvanquished.net/pkg"
builtin in_protoUrl "unv"

builtin in_name "Xonotuc"
builtin in_baseName "xonotic"
builtin in_master1 "master.xonotic.org"
builtin in_master2 "master2.xonotic.org"
builtin in_wwwBaseURL "dl.xonotic.org/pkg"
builtin in_protoUrl "xon"
set fs_legacyPaks on
set fs_maxSymlinkDepth 1
set r_dpMaterial on
set r_dpBlend on

illwieckz · 2022-02-27T22:16:58Z

Superseded by #588.

mbasaglia requested changes Apr 12, 2017

View reviewed changes

DefaultUser changed the title ~~Add "-game <name>" switch to cmdline arguments~~ Remove hardcoded Unvanquished references Apr 18, 2017

DefaultUser commented May 4, 2017

View reviewed changes

illwieckz mentioned this pull request Jul 10, 2017

Linux: follow XDG guidelines #20

Merged

slipher mentioned this pull request May 14, 2018

Design for game-specific customization and mods #108

Open

illwieckz added the T-Improvement Improvement for an existing feature label Mar 13, 2019

DefaultUser added 4 commits December 1, 2019 14:03

Change some strings to be engine specific, not game specific

42d8efd

Read game information from gameinfo.cfg file

750fd46

gameinfo.cfg needs to be located in libpath

Add a template for gameinfo.cfg

f91acbf

Fix mistakes during cherrypicking relevant commits from game_switch b…

bdb3c5b

…ranch

DefaultUser force-pushed the DefaultUser/game_switch branch from 4dd5ada to bdb3c5b Compare December 1, 2019 15:37

illwieckz reviewed Dec 1, 2019

View reviewed changes

src/engine/qcommon/gameinfo.h Show resolved Hide resolved

illwieckz added the T-Cleanup label Dec 13, 2019

DefaultUser added 2 commits December 27, 2019 15:53

Mark most Gameinfo methods const

c7c2299

Search for gameinfo.conf instead of gameinfo.cfg

7b30171

In scope of this engine .cfg files are filled with commands. The Gameinfo file does not contain commands

illwieckz reviewed Dec 27, 2019

View reviewed changes

illwieckz reviewed Dec 28, 2019

View reviewed changes

illwieckz changed the title ~~Remove hardcoded Unvanquished references~~ WIP: Remove hardcoded Unvanquished references May 31, 2021

illwieckz marked this pull request as draft May 31, 2021 22:06

illwieckz mentioned this pull request Sep 27, 2021

Hardcoded references to Unvanquished in the engine Unvanquished/Unvanquished#817

Open

illwieckz mentioned this pull request Feb 27, 2022

Read game information from daemon.ini file #588

Draft

1 task

illwieckz closed this Feb 27, 2022

Conversation

DefaultUser commented Apr 12, 2017

Uh oh!

mbasaglia commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DolceTriade commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbasaglia commented Apr 12, 2017

Uh oh!

illwieckz commented Apr 12, 2017

Uh oh!

Kangz commented Apr 12, 2017

Uh oh!

mbasaglia commented Apr 12, 2017

Uh oh!

illwieckz commented Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DefaultUser commented Apr 18, 2017

Uh oh!

DolceTriade commented Apr 18, 2017

Uh oh!

DefaultUser commented Apr 18, 2017

Uh oh!

Kangz commented Apr 18, 2017

Uh oh!

illwieckz commented Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illwieckz commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

origintopleft commented Oct 11, 2017

Uh oh!

illwieckz commented Oct 11, 2017

Uh oh!

MarioSMB commented May 13, 2018

Uh oh!

slipher commented May 14, 2018

Uh oh!

illwieckz commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

DolceTriade commented Apr 12, 2017 •

edited

Loading

illwieckz commented Apr 12, 2017 •

edited

Loading

illwieckz commented Apr 30, 2017 •

edited

Loading

illwieckz May 24, 2017 •

edited

Loading

illwieckz May 13, 2018 •

edited

Loading

illwieckz commented Jul 10, 2017 •

edited

Loading

illwieckz commented Oct 21, 2018 •

edited

Loading

illwieckz Dec 27, 2019 •

edited

Loading

illwieckz Dec 28, 2019 •

edited

Loading

illwieckz Dec 28, 2019 •

edited

Loading

illwieckz Dec 28, 2019 •

edited

Loading

illwieckz Dec 28, 2019 •

edited

Loading

illwieckz commented Jan 19, 2020 •

edited

Loading