News:

Simutrans Wiki Manual
The official on-line manual for Simutrans. Read and contribute.

DSG Development Blog

Started by DrSuperGood, October 11, 2016, 12:52:34 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

DrSuperGood

This is a front for feedback, suggestions and goals relating to my work on Simutrans. It is intended to add some transparency and thought process behind the making of changes and hopefully document them for future developers to better understand why something was done.

Bug fixes I make will not be covered here because they are documented by the bug report thread itself.

My current aim is to revise the logic and inner workings of several in-game tools players can use. These changes are being based on a combination of personal experience and feedback from multiplayer server administrators and players.

A big recurring complaint is the sacrificial company exploit. Players who's company cannot afford an expensive permanent action would start a new dummy company and then bankrupt it doing the action while their main company reaps the rewards from the action. Most commonly seen with terraforming and city building etc. This was especially a problem on servers where city building was meant to be disabled as sacrificial companies could be used to build such cities even if their price was intended to be impossible.

To combat this fund checks are being added to many of the tools. So far these include plant trees, build city, build artificial wall/incline, build headquarters and make public. These actions can no longer be performed if a company has insufficient funds (in the form of equity) for the action to fully complete. Although this will not stop actions such as plant tree, it will stop people building cities if they are set to a high enough price as well as prevent them from accidently bankrupting themselves.

The complexity trees add has been a bane for server owners for some time due to increased map transfer times. This is why most server games have the "no_trees" configuration flag enabled so random maps are generated without any trees on them. Although they might start out treeless, players can still add trees to them possibly out of malice. Since trees grow and spread, a few trees planted around the map can end up into massive forests after 200 years or so which do have noticeable effects on map size. To stop this the no_trees option now completely disables planting of trees, even by the public service player.

The make public tool can be very useful in server games for making or rebuilding public ways which are useful for other players to use but not used by the responsible company. However the tool has several strange mechanics to it such as not functioning on bridges or inclines. Ways built on inclines before required some hacky artificial wall work to be made public which cost the player extra and tunnel entrances were just impossible to be made public. This has been changed so that the tool will now also function on ways that are built on an incline, including tunnel entrances. Bridges still cannot be made public however it is something I plan to eventually look into.

The cost of the make public tool was unclear, lacked the ability to be configured and seemingly random. This has been changed so that it now costs a configurable number of months of maintenance (default 60 or 5 years of operation) which is then transferred to the public player. Public player takes ownership of ways for free. As mentioned above it also gets fund checked hence setting the configuration to something ludicrous such as 36,000 months (3000 years) could in theory make its use impractical/impossible.

A piece of code was removed from the make public stop method that seemed to be dead. After many tests in practically every conceivable station join configuration from both players and public service provider using the tool the branch of code was never executed and hence why I am fairly certain it is dead. I am guessing it was left in from when the method used to join any two stops and make them public and hence had to cope with another non-public stop rather than assuming it is already public which is what the current implementation forces.

The tool error string translation constants are being placed in string constants. This is for maintainability as otherwise the same string constant ends up declared in multiple locations so is hard to change and can be error prone with typos or copy mistakes.

I plan to apply my last JIT2 revision at some stage after updating it for compatibility before going on to see if the problems with JIT2 can be ironed out. I also intend to hack around with more of the tools, revising code and adding fund checks. I am open to suggestions for minor things as well.

Combuijs

Quote
To combat this fund checks are being added to many of the tools

Hopefully "-freeplay" players still can use those tools even when in great debt?
Bob Marley: No woman, no cry

Programmer: No user, no bugs



DrSuperGood

#2
Quote

Hopefully "-freeplay" players still can use those tools even when in great debt?
Ofcourse they can :). Assuming that the freeplay option is selected then all finance checks pass. Additionally the public service player always passes finance checks even when not in free play.

prissi

I still find treeless maps boring, so manually adding a few trees is more beautiful. Maybe instead forbidding tree creation on your maps, decoupling tree planting and tree removal cost could forbid planting trees just by making them too expensive while removal was still possible.

A map without trees would be treeless until the public player adds the trees. SO both side could do without forcing no_trees = never trees.

jamespetts

One could perhaps forbid players other than the public player from planting trees in a network game? Transport companies do not usually plant trees, after all.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

DrSuperGood

#5
Quote
I still find treeless maps boring, so manually adding a few trees is more beautiful. Maybe instead forbidding tree creation on your maps, decoupling tree planting and tree removal cost could forbid planting trees just by making them too expensive while removal was still possible.
One can turn the flag off after creating the map allowing them to be planted (not a problem in single player). I checked this and can confirm it works. Some server owners with faster servers or small maps do not mind the trees so keep the flag off anyway. It is mostly aimed at multiplayer where server owners purposely want no trees to optimize save/load sizes and I have seen trees used to troll/vandalize to some extent (no other reason why they were planted individually over a huge area of the map).

Some exception could be added for the public service player however I feel this would degrade the meaning of the flag. Perhaps adding "no_trees_spread" flag so that they do not naturally grow might be more useful in such a case? One can leave no_trees off and not have to worry about huge forests eventually showing up everywhere.

The main problem is the tree spread, since even 1 tree after a few hundred years can cover a significant area with trees. Each tree tile can only be deleted 1 tile at a time so cleaning up from a player who purposely planted trees everywhere in a no trees server is not viable effort wise.
Quote
One could perhaps forbid players other than the public player from planting trees in a network game? Transport companies do not usually plant trees, after all.
It really depends on the server though. Some people have no problems with trees at all while other owners do.

prissi

One tree cannot spread, you need at least three of them ... or so was the intention of the algorithm.

DrSuperGood

Public service player can now build trees even when no_trees is set.

I also pulled out some more of the translator text identifiers into constants. The general intent it to make the code more maintainable as one can easily see what translated text is available and be sure not to make any typo errors when referencing one or accidently creating a new reference for text that already exists. Since the identifier string is in one place it can be changed fairly easily as well, not that there is a reason to do so. Another advantage is that each text identifier can be commented as to its purpose and meaning, which is useful for those text identifiers which were written in German that some people might have problems understanding (abstracts away the German). Eventually it might be a good idea to pull them out into a separate header file that is included every where translated text is needed by the engine.

TurfIt

Quote from: DrSuperGood on October 11, 2016, 12:52:34 AM
The complexity trees add has been a bane for server owners for some time due to increased map transfer times.
The default forest settings for both pak64 and pak128 tend to create too dense of forests IMHO. A few parameter tweaks and the created forests are much better suited for online games - 1/2 the initial file size and still looks good.

But, the real problem is after ~80 years the entire map suddenly get filled with trees - surely this is a bug, but I've not had a chance to track down...


Quote from: DrSuperGood on October 11, 2016, 12:52:34 AM
The make public tool can be very useful in server games for making or rebuilding public ways which are useful for other players to use but not used by the responsible company.
No. The tool is called make_stop_public. It should only work for it's original purpose to create public interchange stations. The ability to use it on ways, etc. seems an oversight and should be completely removed to eliminate the exploit of players ending up with no maintenance costs. If necessary, a second tool to create public ways etc could be created, but must be disable-able for a sane multiplayer game.

Related issue: when a player exploits a shell company to make all the ways public, and the shell goes bankrupt, the ways end up owned by 'null' instead of the public player, hence tolls don't even apply! Nice to fix that too...

Also, changes of this magnitude to game mechanics should ideally have a patch posted for comments first.


Quote from: DrSuperGood on October 11, 2016, 12:52:34 AM
The tool error string translation constants are being placed in string constants. This is for maintainability as otherwise the same string constant ends up declared in multiple locations so is hard to change and can be error prone with typos or copy mistakes.
Again, such a change should IMHO have had some discussion first... makes it harder to read as you now have to find the string defined somewhere other than where it is used to see what it actually says.

Changes to translated strings are still all but impossible. Simutranslator is a mystery, and whoever admins it for adding new accounts doesn't respond (still waiting after 10+ months...) 
I.E. your change to the make_stop_public tooltip breaks (orphans) all existing translations. Also, the base.tab file wasn't updated with the new text.
The added "Trees disabled!" also wasn't put into base.tab.


Quote from: DrSuperGood on October 11, 2016, 12:52:34 AM
I plan to apply my last JIT2 revision at some stage after updating it for compatibility before going on to see if the problems with JIT2 can be ironed out. I also intend to hack around with more of the tools, revising code and adding fund checks. I am open to suggestions for minor things as well.
I know I'm still negligent at getting to it. If it only affected JIT2, then go ahead. But it affects electricity overall in JIT0/1 as well, and there was just something in the changes that gives me a gut feeling of being wrong. I'll get there...

---
The added fund checks are good.  (atleast I can get one good comment into my bloody negative post)

DrSuperGood

#9
Quote
No. The tool is called make_stop_public. It should only work for it's original purpose to create public interchange stations. The ability to use it on ways, etc. seems an oversight and should be completely removed to eliminate the exploit of players ending up with no maintenance costs. If necessary, a second tool to create public ways etc could be created, but must be disable-able for a sane multiplayer game.
It is no oversight. Code was purposely written to make ways public long ago.

There is a setting to adjust its cost. To make a way public people have to pay by default 60 months maintenance (5 years) to the public service player. This can be quite substantial for pak128. If it is set to some insane value like 36000 (3000 years) then people will either not be able to afford the tool or if they can then the public service player gets enough money to maintain the way for 3,000 years so it is not free.

Quote
Related issue: when a player exploits a shell company to make all the ways public, and the shell goes bankrupt, the ways end up owned by 'null' instead of the public player, hence tolls don't even apply! Nice to fix that too...
Its on my to do list. Also city roads to be owned by public (not null) so they charge toll and to force city roads near public buildings (stops replacing them with fast roads in cities, all those kids playing in the streets will be safe!).

Quote
Also, changes of this magnitude to game mechanics should ideally have a patch posted for comments first.
I have discussed them with people online for a while. Including some other devs, people like fifity and several server owners. Also I made this thread to discuss such changes and gather feedback for that very purpose.

If one wants Simutrans to progress ultimately things need to get done rather than only talked about.

Quote
Again, such a change should IMHO have had some discussion first... makes it harder to read as you now have to find the string defined somewhere other than where it is used to see what it actually says.
However the constants are given a meaningful English name. Also the problem existed before except you had to search through a 5 thousand lines of source looking for what strings have been declared, or go to Simutranslator and look them up there and make sure to copy them perfectly.

Quote
I.E. your change to the make_stop_public tooltip breaks (orphans) all existing translations. Also, the base.tab file wasn't updated with the new text.
I tested it in game and translations appeared to still be working. I was not aware of the base.tab file, will look into it. The same strings are used (except the ones I added), just they are stored in a constant which is referenced rather than having multiple copies spread around over 5 thousand lines of code.

Quote
I know I'm still negligent at getting to it. If it only affected JIT2, then go ahead. But it affects electricity overall in JIT0/1 as well, and there was just something in the changes that gives me a gut feeling of being wrong. I'll get there...
Well if something is wrong then it can be fixed. I ran tests with it and no major crashes or errors occurred. Simutrans is not perfect as is, hence all the bug reports and fixes. Although it is a good idea to try and avoid introducing new bugs and errors, if progress is to be made then ultimately new errors or bugs might be introduced. After all that is how the existing ones which get fixed made their way into the game in the first place. Ah the development cycle :)

An_dz

Quote from: DrSuperGood on October 13, 2016, 02:12:58 AM
I tested it in game and translations appeared to still be working. I was not aware of the base.tab file, will look into it.
The base.tab file is just for importing the strings to Simutranslator, it does not affect the translation mechanics.

River

Quote from: DrSuperGood on October 11, 2016, 12:52:34 AM
The cost of the make public tool was unclear, lacked the ability to be configured and seemingly random. This has been changed so that it now costs a configurable number of months of maintenance (default 60 or 5 years of operation) which is then transferred to the public player. Public player takes ownership of ways for free. As mentioned above it also gets fund checked hence setting the configuration to something ludicrous such as 36,000 months (3000 years) could in theory make its use impractical/impossible.
This seems like a good start but 5 years of opperrating cost is not alot if a player is gone use a massive tunnel for a 100 years. And if you are gone set the cost to 100 years it might be to expansive to share a (train) station where it is needed when a new player is starting. I should rather see a way to have the player that build the shared object to keep paying opperrating cost.

you would still be able to get around the opperrating cost by bankrupting companies unless everything shared is removed when a company goes bankrupt.

I'm wondering if this idea could work?

jamespetts

You could just adopt the system in Experimental, where ways and stops can be made public (nationalised) only by the public player, but players can connect their lines and give each other permission to run on each others' lines and stop at each other's stops.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Tjoeker

I don't play online, so I'm not really sure, but isn't it mostly impossible to switch to the public player?

maybe a checkbox 'share station' could do the trick?
just like the checkbox allow city growth in the city dialog.

Ters

This is becoming a tangle of different topics, but I think that if a stop is made public, which means the player no longer has to pay for its upkeep, then the players should have to pay to use it.

DrSuperGood

#15
Quote
This seems like a good start but 5 years of opperrating cost is not alot if a player is gone use a massive tunnel for a 100 years. And if you are gone set the cost to 100 years it might be to expansive to share a (train) station where it is needed when a new player is starting. I should rather see a way to have the player that build the shared object to keep paying opperrating cost.

you would still be able to get around the opperrating cost by bankrupting companies unless everything shared is removed when a company goes bankrupt.

I'm wondering if this idea could work?
Quote
You could just adopt the system in Experimental, where ways and stops can be made public (nationalised) only by the public player, but players can connect their lines and give each other permission to run on each others' lines and stop at each other's stops.
Public ways and stops are not a problem. In fact they are very useful for things like public roads between cities. The tool is needed so that a player can modify public roads (those which the map generator places, or even ones that it does not) around their ways like rails without having to forcefully take ownership of the way preventing it from being upgraded etc.

As a counter example of why the experimental system is bad. In the server of the last release, players made a lot of rails and cannals everywhere which had to be bridged over. Since they owned those bridges and roads it meant that only they could upgrade them. Since the bridges used lacked the weight loading for cars the result was a mess of islands that could not be driven between by cars despite there being a ton of bridges. If these were made public and could be modified by the players then the player who wanted to use a connection could personally upgrade the required bridges without having to beg the other company to do so and then waiting several days for a response.

Quote
This is becoming a tangle of different topics, but I think that if a stop is made public, which means the player no longer has to pay for its upkeep, then the players should have to pay to use it.
Maybe a system of returning non-shared assets? Such as if the station is not shared by at least 2 companies after 5 years (only the person who built it is using it) then it reverts back to them?

Another approach might be to improve tolling for the public player. It sums up some kind of usage metric for each player which it then charges them for at the end of month.

Alternatively some kind of taxation system could be used where players have to pay for the public player maintenance based on the fraction of total transported.

I think a lot of public way and stop abuse cases are more a job for server moderating than having the feature removed entirely. There are cases for fair use of the tool. However based on feedback here I will look into adding configuration flags to disable the tool working on ways (except if used by public service provider) .

If mixed stop ownership was allowed (where the same stop can be composed of buildings owned by different players) then public stops could be removed entirely. This would even be a usability benefit since then each player can modify the parts they use (unlike now where public stops can only be modified by the public service player and Experimental's approach where only the owner can modify and expand a stop which is very inconvenient to other companies). However I doubt this is a simple job despite ownership being tracked on a per building level as a lot of the code probably presumes a single owner.

jamespetts

Quote from: DrSuperGood on October 13, 2016, 03:40:27 PM
As a counter example of why the experimental system is bad. In the server of the last release, players made a lot of rails and cannals everywhere which had to be bridged over. Since they owned those bridges and roads it meant that only they could upgrade them. Since the bridges used lacked the weight loading for cars the result was a mess of islands that could not be driven between by cars despite there being a ton of bridges. If these were made public and could be modified by the players then the player who wanted to use a connection could personally upgrade the required bridges without having to beg the other company to do so and then waiting several days for a response.

The difficulty with allowing a non-public player to make something public is that that can amount to seizure of assets of another player. This could allow vandalism of others' networks. Further, if the making public tool were made so expensive as to discourage its use to reduce costs (such as using a long tunnel for a century), then it would likewise be too expensive for players to deal with the issue that you describe by seizing other players' ways.

Experimental has been changed since the last large online game to allow players to alter roads built on map generation, which are now null player rather than public player, but makes them all public rights of way, meaning that they can only be upgraded (not downgraded), anything that they are upgraded with will also be a public right of way (and thus passable by any player's vehicle), and can only be removed if a short diversionary route (which, on deletion of the original way, becomes a public right of way itself) be provided. This also works for rivers. This effectively deals with the issue so far as roads and rivers created on map generation are concerned.

Quote
Alternatively some kind of taxation system could be used where players have to pay for the public player maintenance based on the fraction of total transported.

Taxation is a very interesting idea and one that has been considered for Experimental for some time; it would make more sense to have a realistic tax system in which taxes are based on a proportion of profits.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Ters

Due to the lack of sufficient artificial intelligence, it seems clear to me that someone must take on the role of the public player to provide the common infrastructure. Realistically, this would be pretty much all intercity roads. Depending on era, it would also include airports, stations and bus terminals. Whether such realism makes for good game play, especially for the one playing the public player, is a good question.

prissi

@river
Since the cost check would apply to make things public, then making a massive tunnel network public will not work too.

DrSuperGood

Quote
Experimental has been changed since the last large online game to allow players to alter roads built on map generation, which are now null player rather than public player, but makes them all public rights of way, meaning that they can only be upgraded (not downgraded), anything that they are upgraded with will also be a public right of way (and thus passable by any player's vehicle), and can only be removed if a short diversionary route (which, on deletion of the original way, becomes a public right of way itself) be provided. This also works for rivers. This effectively deals with the issue so far as roads and rivers created on map generation are concerned.
The problem had nothing to do with pre-placed bridges and roads, but rather the roads built by players to bridge over their ways. Since ways intersected the map everywhere dividing it into a grid of sorts players owned a lot of such overpasses. As they never bothered to upgrade these or could not make them publically modifiable the result was a mess of roads no one could use.

To stop the proxy company problem I have suggested tracking player IP addresses. Servers can then limit company creation to 1 per unique IP. Although this will not stop people with dynamic IP addresses from making more companies and will stop multiple players who share a NAT from doing so, it would make it largely inconvenient to exploit the trick. A setting for such a system would be useful as some server owners might object to it.

Generally exploiting public ownership to save on maintenance is kind of pointless in standard. In most server games I play for both pak64 and pak128 I end up making so much profit that I could completely absorb the public service player and still be sitting in tons of profit. Often I am not alone, with at least 3-4 other people ending up in a similar situation. Hence why I think exploiting of the tool should be more an act of moderating the server rather than physically limiting it.

When I use the tool its because I needed to change a public way for my own ways so I moved the way in some form (eg onto a bridge, or into a tunnel). I do not want to own that, because later on (eg pak128 where fast roads get introduced later) someone might want to upgrade it when they use it. Many servers have run rules like "repair all public ways" etc.

Quote
Due to the lack of sufficient artificial intelligence, it seems clear to me that someone must take on the role of the public player to provide the common infrastructure. Realistically, this would be pretty much all intercity roads. Depending on era, it would also include airports, stations and bus terminals. Whether such realism makes for good game play, especially for the one playing the public player, is a good question.
Having the public service player as an actual player, instead of some kind of super administrator, has been in my head for a while. Although I think it could bring some interesting game play decisions and could even add to servers (if he goes broke, you lose), the problem is that it would need major game play changes to implement. So for now is probably not worth thinking about.

River

Quote from: prissi on October 13, 2016, 08:35:21 PM
@river
Since the cost check would apply to make things public, then making a massive tunnel network public will not work too.
You can still use multiple companies to connected the tunnels to create a big one.

Quote
Due to the lack of sufficient artificial intelligence, it seems clear to me that someone must take on the role of the public player to provide the common infrastructure. Realistically, this would be pretty much all intercity roads. Depending on era, it would also include airports, stations and bus terminals. Whether such realism makes for good game play, especially for the one playing the public player, is a good question.
Having the public service player as an actual player, instead of some kind of super administrator, has been in my head for a while. Although I think it could bring some interesting game play decisions and could even add to servers (if he goes broke, you lose), the problem is that it would need major game play changes to implement. So for now is probably not worth thinking about.
I think this would be a interesting option, i would love to play as the public player. i wouldn't put it in as a default setting thought.

jamespetts

I should hope that the way wear mechanism would help with automatically upgrading thoroughly obsolete ways. In any event, this sort of problem is not otherwise resolvable without giving opportunities for players to seize and/or vandalise other players' networks, as far as I can make out, at least not without super-intelligent A. I..
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

DrSuperGood

The ways of liquidated companies should now go to public player as opposed to NULL. There may be a logic error with the maintenance for tunnel and bridge ownership transfer when under a public stop. There should be methods that automatically manage ownership change and maintenance rather than every location it occurs having to do so with the same several function calls. There might also be a minor graphic bug as I do not see any of the ownership changed objects being made dirty to be redrawn.

DrSuperGood

#23
I spent an hour odd restructuring the make public tool code. Looking at the logic there was a potential NULL dereference in there if the position input did not represent a valid ground object, but I am unsure if it would have ever been able to occur.

Way objects are now treated separately from ways and are chosen if no suitable stop or way exists to be made public. The reason for this was to stop obscure situations where one could build privately owned way objects on public ways and not make those objects public yet if one made them on a privately owned way and made that way public they would also become public. This was made worse by a company being able to build a stop on the public way while it had a different companies way object on it.

I really want to add logic to make bridges public. Seeing as they have to be done as a single transaction of all bridge components it might need its own special case logic.

DrSuperGood

Added basic drag support to the make public tool. Dragging apparently was implemented before, but did nothing useful as far as I could tell. I am guessing it was meant to mark an area to be made public, displaying a cost preview, and then when released make the entire area public. Instead it showed some costs which would follow the tool around in a buggy way and do nothing when released. The new drag behaviour is similar to building stops, addons or any such drag and it does work tools.

To prevent the public way notification messages from generating a lot of spam on multiplayer servers similar messages are now discarded. A message will now only show once for every 32*32 area within the last 20 messages. This will mean upgrading a sizable stretch of road to public would generate at most a few messages, rather than hundreds. This change was made to counteract the possible spam the new drag support could generate as it is now much easier to make long stretch public very quickly and each tile would otherwise generate a message. The messages were kept as it can help people keep track of what is happening when away from multiplayer server games.

At some stage it might be recommended to add a proper message filtering system rather than hacking in filters into the add message function. This is because such hacks are adding specific and possibly unexpected behaviour to the function which could lead to errors in future (people wondering why certain messages do not show).

Currently localization is a big issue with multiplayer Simutrans. Everything is localized only on creation, using the current simutrans locale. This is fine for singleplayer where one does not expect to change localizations often. However for multiplayer it results in all clients who join being sent a map localized into the server's locale with all new events occurring since joining being localized into the client locale until they rejoin. Not only is this inconsistent but also is bad for usability as people end up seeing a mixture of various locales. For example if an English player currently was to join a Japanese server running a pak which featured Japanese translation then all factory names, convoys, stops, previous game messages etc would be in Japanese even though they are using standard names. What should happen is that all default (not user changed) names are saved as such and then localized to the client locale on load.

The most complex and efficient way to implement this would be to create some kind of abstract string stream which concatenates the results of different abstract functional string units into a concrete string which gets displayed to the client. Names of factories are stored as such a stream rather than as a concrete string. This could potentially save space as storing a default factory name could take as little as 1 byte (special value describing the objects default name).

Another more simple way would be to use some kind of string markup language to represent translatable strings. In such a case the language allows one to store translatable references which when evaluated expand to the localized string. For example a factory of type BookFactoryA would store a string like "{BookFactoryA}" rather than "Printing Works" which still gets displayed the same in game. For formatted translatable strings which get saved (eg some notification messages) the formatted arguments would need to be either packed into the language, or stored separately in a list which accompanies the internal string. This has the obvious advantage that users could use these abstract translatable references in custom names, for example line name "{BookFactoryA} In" could be automatically localized to "Printing Works In".

If I will look into implementing such a thing I am still thinking about. Until then I will look into making powerlines and/or transformers public, possibly even with a configuration to force it. Reason for this is that powerlines on multiplayer games can become a big hazard and companies often have inefficient networks due to ownership boundaries and no way to join them without help from the public service player.

DrSuperGood

I happened to stumble upon the network core of Simutrans and noticed the sockets were implemented incorrectly. The structure length resolution was violating the sockets API as it was using hard coded constants instead of the dynamic value assigned in the lookup structures. Local addresses were not being enumerated properly, trying to bind the same address for every iteration. Added support for service names instead of port numbers to network URNs.

A URI parser might be recommended at some stage instead of a hacky string parser.

Why do clients have declarable local addresses for sockets? The sockets API should automatically find a local address for a socket to connect from without the need for one to be given explicitly.

What uses the IPv4 only code? As far as I am aware Simutrans minimum supported Windows is XP which did support IPv6, and Linux/Mac has supported IPv6 for ages now. Even if IPv6 is not supported the sockets API should still work, although only returning and using IPv4 addresses. It might be time to remove all the IPv4 only code to improve maintainability of the network core.

As this was a substantial change to the network core please report if there are any Linux compile errors. I can only guarantee the build works on MSVC2015.

Dwachs

Could you please post patches for review before doing such substantial changes?

I could not look into it, as the changes did not propagate to github yet. At leastm the nightly server succeed to compile r**24.
Parsley, sage, rosemary, and maggikraut.

jamespetts

Dr. Supergood - is there any possibility that this might be related to the desyncs between Windows and Linux clients on recent builds of Experimental?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Vladki

r7924 compiled on linux, without any problems, connect to "regional" game OK.

DrSuperGood

Quote
Could you please post patches for review before doing such substantial changes?
This change was hardly substantial though. It was mostly fixing already existing errors in the socket logic. Hence maintenance work.

Quote
Dr. Supergood - is there any possibility that this might be related to the desyncs between Windows and Linux clients on recent builds of Experimental?
Unfortunately not. The network core was working because it was correct enough to work. It would only not work under some obscure, probably unlikely, situations. For example if a protocol other than IPv4 or IPv6 was returned from a name resolution provider it would fail due to incorrect address structure size. Or if the first local address found failed to bind to the socket then it would keep retrying to bind that address each iteration rather than trying any others, if any, that were found. The ability to resolve port services as well as port numbers was the result of treating the destination port as a string rather than pointlessly converting from string to integer, which loses all service names, then from integer back into a port string.

Quote
r7924 compiled on linux, without any problems, connect to "regional" game OK.
Thanks for confirming. Although the code should have worked (I made sure to read both Linux and Windows socket APIs) there was always the possibility of some obscure problem. However I guess everything is good.

jamespetts

Thank you for clarifying that - it is helpful.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Ters

Quote from: DrSuperGood on October 30, 2016, 03:22:06 PM
This change was hardly substantial though. It was mostly fixing already existing errors in the socket logic. Hence maintenance work.

Maybe one of you are thinking qualitatively and the other quantitatively. If a lot of lines are changed, it can be difficult to be sure that the changes are just a simple fix, and that no other changes have slipped through, intentionally or unintentionally. That is when I appreciate having a good set of tests verifying behavior during every build, even if I sometimes hate writing those tests in the first place and the tests take a long time.

DrSuperGood

Quote
That is when I appreciate having a good set of tests verifying behavior during every build, even if I sometimes hate writing those tests in the first place and the tests take a long time.
I do run through a variety of user level tests for every change I make. Firstly I make sure the code compiled with MSVC2015 (which does not mean GCC unfortunately, hence why an occasional error slips through). I then make sure that I get vaguely what I expect from the area of the change, in this case the multiplayer tab of the main menu. At first I did not, but it turned out to be URN parser code changes I made due to some obscure stuff which was done with the old code, hence the migration to std::string which also solved some other problems such as maximum name length. After it was working I then joined the regional server, which should be sync compatible at the moment due to no changes with the game results, and waited to see for any issues. As the joining worked and the game was stable for a long time I assumed the changes were correct. On top of that I tried to querry a variety of servers to check both name resolution and service resolution were working. The results were as expected, with port numbers making a difference to finding or not finding a server. The service resolution results are still not strong as no server currently uses a service name for its port (I only can assume it works based on the socket specification for resolving port numbers).

Ters

My point was that with good test coverage, one does not have to just trust each developer to manually test every possible case. Any change in behavior can often also be seen much clearer in the tests, making reviewing the changes much easier. Of course, one has to obtain good test coverage first, which is no small feat for a complex system.

kierongreen

DrSuperGood, I appreciate that you, as every developer, endeavours to fully test changes before committing. However unless these are entirely trivial (a handful of lines at most) as Ters says, there is the possibility of unintentional changes occurring. We all try to write good code, sooner or later we will all make mistakes. Bugs may happen which result in intermittent issues and peer review minimises the possibility of this.