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

UDP query of game script #9234

Closed
wants to merge 4 commits into from
Closed

UDP query of game script #9234

wants to merge 4 commits into from

Conversation

@TheDude-gh
Copy link
Contributor

@TheDude-gh TheDude-gh commented May 10, 2021

Reopening the idea of UDP query of game script and probably showing it also in game GUI.

This patch focuses to extend fetching game info over UDP from multiplayer servers. Currently you can get basic game information and NewGRF details, but nothing about game scripts.

I think it would be useful to have possibility to found out, if server is running game scripts. When servers has a game scripts, it and could be shown in ingame lobby window and could be added also to web server listing .

Currently this patch solves it by adding new packet type, so there is extra UDP communication needed, but I think it would be better to rewrite it a little bit and incresase NETWORK_GAME_INFO_VERSION and add it to the end of the PACKET_SERVER_GAME_INFO packet in SerializeNetworkGameInfo() in game_info.cpp.

TheDude-gh added 2 commits Oct 15, 2019
Reopened from old flyspray issue OpenTTD#6211.

Patch adds a possibility to query OpenTTD server for running game scripts and returns its name and version if any present.

Goal of this fork is to make also ingame interface.
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 10, 2021

Honestly, I am not really sure what to do with this.

You mention that you want to reopen an idea, but you don't start a discussion; you just show some code. You really have to explain a bit more.

The Pull Request template we have helps you with that. It tells you to write a motivation and a description of your Pull Request. But as you didn't do that, you didn't "reopen" anything really. You just showed us some code.

Please help us help you. Don't make it our work to figure out what your intentions are, what you want to accomplish, etc. That really is your job: convince us why we should want this. And for that we have a template, to guide you through that process.

Could you please fill in the template and help us out what is the idea here?

Thank you!

Loading

@TheDude-gh
Copy link
Contributor Author

@TheDude-gh TheDude-gh commented May 10, 2021

I have probably missed the template, my bad.

This patch focuses to extend fetching game info over UDP from multiplayer servers. Currently you can get basic game information and NewGRF details, but nothing about game scripts.

I think it would be useful to have possibility to found out, if server is running game scripts. When servers has a game scripts, it and could be shown in ingame lobby window and could be added also to web server listing .

Currently this patch solves it by adding new packet type, so there is extra UDP communication needed, but I think it would be better to rewrite it a little bit and incresase NETWORK_GAME_INFO_VERSION and add it to the end of the PACKET_SERVER_GAME_INFO packet in SerializeNetworkGameInfo() in game_info.cpp.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 10, 2021

What I miss in your description, is a motivation. What do you want to accomplish with this?

I can see it adds a packet that gives some GameScript information. But why specific that information? And why that alone?

In other words: how do you intend to use it? What is the request that made you build this? Just that you find it useful is not really a convincing argument :) Please elaborate a bit more :) Tnx!

(edit: at least, I assume you didn't add a full packet just for the game script name; that could be done in the GameInfo too. Given the amount of data, I assume you have a higher purpose here. But this is my interpretation, so a bit more explaining how you want to use this would really help)

Loading

@TheDude-gh
Copy link
Contributor Author

@TheDude-gh TheDude-gh commented May 10, 2021

I have two motivations:

  1. When playing multiplayer, you could look if there is specific game script on some of the servers (that means a filter option for game scripts would have to be included in patch).
  2. A search over all servers could be done to have statistics about game scripts used, a similar thing I have done for newgrfs some time ago.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 10, 2021

Tnx!

In that case this PR needs a bit more work of course :)

Starters, I would add this to GameInfo. It is just a simple string, possibly with a version, so that would make two simple strings, or maybe merge it into a single one ("name version" or something). It means we don't need a new packet, which is always a bonus.

But mostly, this PR would also need to implement the usage of this packet. We tend not to like stuff that just adds something that someday someone might use .. those things tend to be forgotten, and break :P

Did you have any thought on how that would work in the GUI?

Personally, I am not against something like this; but this currently is just an idea that I guess needs to be worked out a bit further :)

PS: if we do this for GS, we might as well do it for AIs too, but that is another story :)

Loading

@TheDude-gh
Copy link
Contributor Author

@TheDude-gh TheDude-gh commented May 10, 2021

My idea of GUI is to have new text about GS in the right pane with game info, when server is selected.

And new filter option for game script somewhere in the area where current server name filter is.

Schránka 02

But I am not really sure, whether to add the filter right away. Maybe I would start with adding just the UDP info and the simple new row, and when it is included, we could see how much are GS actually used across multiplayer games. And just after that add new filter.
Depends on the opinions from development team.

As for AI information, I would'n mix it with this patch. Are AI's even used in multiplayer games? While I can easily imagine people would search for specific GS, I am not so convinced if anyone would be interested in server with AI's.

Loading

New version of gameinfo packet with extended detail about gamescript instead of new packet type
Copy link
Member

@TrueBrain TrueBrain left a comment

A lot better, tnx :) Now it also makes a lot more sense what you are doing :D

Loading

src/network/network_udp.cpp Outdated Show resolved Hide resolved
Loading
src/network/network_udp.cpp Outdated Show resolved Hide resolved
Loading
src/network/network_gui.cpp Outdated Show resolved Hide resolved
Loading
src/network/core/game_info.cpp Show resolved Hide resolved
Loading
src/network/core/game_info.cpp Outdated Show resolved Hide resolved
Loading
src/lang/english.txt Outdated Show resolved Hide resolved
Loading
src/network/core/game_info.cpp Outdated Show resolved Hide resolved
Loading
src/network/core/game_info.cpp Show resolved Hide resolved
Loading
@TheDude-gh
Copy link
Contributor Author

@TheDude-gh TheDude-gh commented May 11, 2021

One thing that bothers me with adding gamesript info to game info packet, and the reason I haven't do that in the older fork, is that it breaks getting game info for any older openttd versions before this patch (if it would be included).

For some reason I do not quite understand new stuff in info packet is added before the old. So when older version reads info packet from new hypothetical version, it will read GS info where it would currently expect NewGRF info, and thus fail.

Solution would be to start adding any new info data to the end of the packet, so when older version reads new version packet, it reads what it knows and throw away the rest. But then it would not be consistent with current coding choice.

What do we about that? I myself would prefer functionality of older versions before consistency of the code.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 11, 2021

What do we about that? I myself would prefer functionality of older versions before consistency of the code.

Partial true what you mention. What happens is this:

Older clients that request data from a newer servers, get "v5". They do not know how to read it, so skip reading it completely (they never hit any case in the switch).
Newer clients that request data from an older server just starts at "v4", and can happily read it.

This is mostly always the case with protocol changes. It is backwards compatible, but not forwards compatible.

That said, we are currently reworking some parts of the networking code, among which the GameInfo. In #9017 it is already bumping GameInfo to v5, and this can just be part of that bump. So v5 is already happening, so there is no reason to resolve it in another way :)

Loading

@TheDude-gh
Copy link
Contributor Author

@TheDude-gh TheDude-gh commented May 11, 2021

True, it will not run into any problems, but it will still display the new version unreadable info in GUI like this:

lobby

So old versions will display lot of this empty rows when there are many new version servers.
I would stick to "people should upgrade their software" motto.

Loading

Removing useles code
Packet sends only version and GS name
Change width of details right pane in lobby window (WID_NG_DETAILS) from 140 to 270px, so even longer strings are visible
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 13, 2021

Tnx!

Most likely I cherry-pick this PR into #9017, or rebase it after that is merged. As that is already bumping game-info version, might as well make it part of the package. But that needs a bit more time, so stay tuned :)

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jun 29, 2021

As promised, I integrated this in #9017. Will close this PR once that PR is merged :)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants