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

Remove hardcoded Unvanquished references #16

Open
wants to merge 4 commits into
base: master
from

Conversation

@DefaultUser
Copy link
Contributor

DefaultUser commented Apr 12, 2017

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

This comment has been minimized.

Copy link
Member

mbasaglia commented Apr 12, 2017

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

#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 };

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

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

@@ -31,6 +31,10 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#ifndef COMMON_SYSTEM_H_
#define COMMON_SYSTEM_H_

extern const gameinfo_t* current_game;

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

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


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

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

Some of these could even become cvars.

This comment has been minimized.

Copy link
@illwieckz

illwieckz Apr 12, 2017

Member

cvars for this kind of stuff sounds good

This comment has been minimized.

Copy link
@Viech

Viech Apr 18, 2017

Member

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

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 18, 2017

Member

The cvar doesn't need the unv suffix

@@ -1929,7 +1929,7 @@ static ircCmd_t CTCP_Version( bool is_channel, const char * )
return ircCmd_t::SUCCESS;
}

return IRC_Send( "NOTICE %s :\001VERSION Daemon IRC client — v\n" Q3_VERSION "\001", IRC_String( pfx_nickOrServer ) );
return IRC_Send( "NOTICE %s :\001VERSION Daemon IRC client — v\n" Q3_ENGINE "\001", IRC_String( pfx_nickOrServer ) );

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

Does anyone know if the IRC code is supposed to work at all?

This comment has been minimized.

Copy link
@DolceTriade

DolceTriade Apr 12, 2017

Contributor

It works.

This comment has been minimized.

Copy link
@illwieckz

illwieckz Mar 4, 2019

Member

IRC is now nuked: ab3f659

@@ -31,9 +31,19 @@ along with Daemon Source Code. If not, see <http://www.gnu.org/licenses/>.
const char TEMP_SUFFIX[] = ".tmp";

// 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;

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

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

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

Looks like this is also what breaks the build.

@@ -285,7 +285,7 @@ You or the server may be running older versions of the game."

#define PROTOCOL_VERSION 86

#define URI_SCHEME GAMENAME_STRING "://"
#define URI_SCHEME va("%s://", current_game->gamename_string)

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

I think this is one of the strings that should be engine-specific, not game-specific.

This comment has been minimized.

Copy link
@illwieckz

illwieckz Apr 12, 2017

Member

I agree, that's a protocol thing, so that's related to the engine. Perhaps unv:// is not enough reusable for a generic Dæmon thing, any suggest?

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

dmn:// ?

This comment has been minimized.

Copy link
@illwieckz

illwieckz Apr 12, 2017

Member

Does the protocol have to optionally announce the game?
like¹ dgp://gg.illwieckz.net:27960/unvanquished

That /unvanquished would be optional, but if you have multiple games running dæmon engine, your game manager can use the binary (xonotic for /xonotic, unvanquished for /unvanquished, would fill the -game switch), if game not installed but daemon from another game installed, run it with that, if that optional /game is not there, same. That would help Dæmon based games with some outdated engine to not break. That would be very useful for games that enables fs_legacypaks by default (like xonotic), ensuring a game client enabling that legacy stuff is started before joining server.

¹ dgp:// for Daemon Game Protocol for example or another variation with Connect Protocol or words like that, meaning it's a protocol to make game clients connecting a server, following Dæmon guidelines, the same way other engine can reuse the DPK guidelines because they are tired of PK3 design flaws if they want. If defined well, other games would be able to reuse the url format even if they do not rely on Dæmon engine. Well, that's a new topic we can have in forum so I will probably not elaborate more about that topic on that thread. 😁

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

Servers would use different port numbers for different instances, and the game logic should be discovered during connection, so I don't think we need a path component to the URI.

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

Also, different game servers would be listed separately by the master.

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

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia Apr 12, 2017

Member

I think we can do without macro declarations for these.

@DolceTriade

This comment has been minimized.

Copy link
Contributor

DolceTriade commented Apr 12, 2017

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

This comment has been minimized.

Copy link
Member

mbasaglia commented Apr 12, 2017

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

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Apr 12, 2017

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

This comment has been minimized.

Copy link
Contributor

Kangz commented Apr 12, 2017

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

This comment has been minimized.

Copy link
Member

mbasaglia commented Apr 12, 2017

That can be enforced by cvars on the client side.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Apr 12, 2017

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 DefaultUser changed the title Add "-game <name>" switch to cmdline arguments Remove hardcoded Unvanquished references Apr 18, 2017
@DefaultUser

This comment has been minimized.

Copy link
Contributor Author

DefaultUser commented Apr 18, 2017

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

This comment has been minimized.

Copy link
Contributor

DolceTriade commented Apr 18, 2017

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

This comment has been minimized.

Copy link
Contributor Author

DefaultUser commented Apr 18, 2017

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

@Kangz

This comment has been minimized.

Copy link
Contributor

Kangz commented Apr 18, 2017

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

This comment has been minimized.

Copy link
Member

illwieckz commented Apr 30, 2017

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.

@@ -285,7 +288,7 @@ You or the server may be running older versions of the game."

#define PROTOCOL_VERSION 86

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

This comment has been minimized.

Copy link
@DefaultUser

DefaultUser May 4, 2017

Author Contributor

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?

This comment has been minimized.

Copy link
@mbasaglia

mbasaglia May 4, 2017

Member

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.

This comment has been minimized.

Copy link
@illwieckz

illwieckz May 24, 2017

Member

@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.

This comment has been minimized.

Copy link
@illwieckz

illwieckz May 24, 2017

Member

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. 😐

This comment has been minimized.

Copy link
@illwieckz

illwieckz May 13, 2018

Member

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.

This comment has been minimized.

Copy link
@illwieckz

illwieckz Dec 1, 2019

Member

@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?

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Jul 10, 2017

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).

@lavacano201014

This comment has been minimized.

Copy link
Contributor

lavacano201014 commented Oct 11, 2017

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

This comment has been minimized.

Copy link
Member

illwieckz commented Oct 11, 2017

wat?

@MarioSMB

This comment has been minimized.

Copy link

MarioSMB commented May 13, 2018

Any news on this @illwieckz @Kangz?

@slipher

This comment has been minimized.

Copy link
Member

slipher commented May 14, 2018

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

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Oct 21, 2018

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 illwieckz added this to To Do in Smokin' Guns Oct 24, 2018
@illwieckz illwieckz added this to To Do in Xonotic Oct 24, 2018
@illwieckz illwieckz added this to To do in UnrealArena Oct 24, 2018
@illwieckz illwieckz removed this from To do in UnrealArena Oct 24, 2018
@illwieckz illwieckz added this to To do in Standalone via automation Oct 24, 2018
@illwieckz illwieckz removed this from To do in Xonotic Oct 24, 2018
@illwieckz illwieckz removed this from To do in Smokin' Guns Oct 24, 2018
@illwieckz illwieckz added this to To do in Xonotic via automation Mar 13, 2019
@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Dec 1, 2019

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

@DefaultUser DefaultUser force-pushed the DefaultUser:DefaultUser/game_switch branch from 4dd5ada to bdb3c5b Dec 1, 2019
@DefaultUser

This comment has been minimized.

Copy link
Contributor Author

DefaultUser commented Dec 1, 2019

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

#include <iostream>


class Gameinfo

This comment has been minimized.

Copy link
@illwieckz

illwieckz Dec 1, 2019

Member

I know it would probably need more work, but is it possible to leverage existing cfg file parsing code?
also some of the variables already exists as cvar we can set as command line option, maybe we can make the engine reading those cvars first and, if command line option is set, override.

Edit: We would have to make sure the keyword that ends in ~/.local/share/<keyword> cannot be modified outside of this cfg file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Standalone
  
To do
Xonotic
  
To do
9 participants
You can’t perform that action at this time.