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: ENABLE_NETWORK switch #7390

Merged
merged 1 commit into from Mar 20, 2019
Merged

Conversation

TrueBrain
Copy link
Member

This switch has been a pain for years. Often disabling broke
compilation, as no developer compiles OpenTTD without, neither do
any of our official binaries.

Additionaly, it has grown so hugely in our codebase, that it
clearly shows that the current solution was a poor one. 350+
instances of "#ifdef ENABLE_NETWORK" were in the code, of which
only ~30 in the networking code itself. The rest were all around
the code to do the right thing, from GUI to NewGRF.

A more proper solution would be to stub all the functions, and
make sure the rest of the code can simply assume network is
available. This was also partially done, and most variables were
correct if networking was disabled. Despite that, often the #ifdefs
were still used.

With the recent removal of DOS, there is also no platform anymore
which we support where networking isn't working out-of-the-box.

All in all, it is time to remove the ENABLE_NETWORK switch. No
replacement is planned, but if you feel we really need this option,
we welcome any Pull Request which implements this in a way that
doesn't crawl through the code like this diff shows we used to.

This switch has been a pain for years. Often disabling broke
compilation, as no developer compiles OpenTTD without, neither do
any of our official binaries.

Additionaly, it has grown so hugely in our codebase, that it
clearly shows that the current solution was a poor one. 350+
instances of "#ifdef ENABLE_NETWORK" were in the code, of which
only ~30 in the networking code itself. The rest were all around
the code to do the right thing, from GUI to NewGRF.

A more proper solution would be to stub all the functions, and
make sure the rest of the code can simply assume network is
available. This was also partially done, and most variables were
correct if networking was disabled. Despite that, often the #ifdefs
were still used.

With the recent removal of DOS, there is also no platform anymore
which we support where networking isn't working out-of-the-box.

All in all, it is time to remove the ENABLE_NETWORK switch. No
replacement is planned, but if you feel we really need this option,
we welcome any Pull Request which implements this in a way that
doesn't crawl through the code like this diff shows we used to.
@@ -220,7 +220,7 @@ bool HandleBootstrap()
if (BlitterFactory::GetCurrentBlitter()->GetScreenDepth() == 0) goto failure;

/* If there is no network or no freetype, then there is nothing we can do. Go straight to failure. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment may need an update ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I was thinking the same. But look a line lower. It is still correct :)

@TrueBrain TrueBrain merged commit e3c639a into OpenTTD:master Mar 20, 2019
@TrueBrain TrueBrain deleted the remove_dos branch March 20, 2019 18:25
var = network.client_name
type = SLE_STRB
flags = SLF_NOT_IN_SAVE | SLF_NO_NETWORK_SYNC
def = NULL
proc = UpdateClientName
cat = SC_BASIC

[SDTC_STR]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this shouldn't've been removed...

PeterN added a commit to PeterN/OpenTTD that referenced this pull request Mar 20, 2019
PeterN added a commit that referenced this pull request Mar 20, 2019
douiwby pushed a commit to douiwby/OpenTTD that referenced this pull request Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants