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

Feature: Multi-tile depots #9577

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

J0anJosep
Copy link
Contributor

@J0anJosep J0anJosep commented Sep 26, 2021

Motivation / Problem

Replacing thousands of old steam trains with monorail replacements is tiresome.
Also, if trains have a "Go to depot X", several trains may be waiting for entering a depot if one train is already entering or leaving the depot.

Description

This PR allows joining several depot tiles for the same vehicle type into one single depot. For joining them, I have tried to use the same criteria as for joining station tiles.

Replacements with different railtypes are enabled with the option "allow replacing rail vehicles with incompatible railtype". The same applies for road vehicles. Then, users can build a monorail depot tile joined to a normal rail type depot and replacements are much easier.
replacement

If trains get stuck when trying to enter a depot, add more tiles to the depot for easing it.
depot_interaction

#8480 adds extended depots (which are not included here) and also contains the commits of this PR.

Limitations

If a hangar or depot has several tiles, user cannot control in which tile a new vehicle is bought. (On the other hand, the hangar window shows all vehicles in all hangars of the airport: I see this as something good, as in this way it is more difficult to miss stopped aircraft in an airport.)

I don't know whether this feature can be accepted. It adds more settings and getting used to this feature takes time.

The PR touches thousands of lines of code. There are some things I am not happy with yet and I don't have much time to revise it. Code reviews of this PR can help me to improve it, as it will hint me which parts I need to work on.

Possible To-do (discarded by now)

  • Check exploit when rearranging train chains. (add cost for moving vehicles inside the depot/move train chain to some tile to make the exploit more difficult). This can also happen when transferring cargo from one train to another using a station. But as loading/unloading takes time, it is difficult to exploit it.
  • When buying a new train vehicle and appending it to an existing chain, keep the tile of the existing chain. Add no cost.
  • When issuing a move train chain command, add a cost for vehicles that teleport (distance + heightlevelincrease).
    Note: leaving the depot at a speed of 0 is a penalty to consider, but maybe not enough for steep landscapes.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? No (label: 'backport requested')
  • This PR touches english.txt or translations? Yes Check the guidelines
  • This PR affects the save game format? Yes (label 'savegame upgrade')
  • This PR affects the GS/AI API? Yes (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? No

@J0anJosep J0anJosep changed the title Multi-tile depots Feature: Multi-tile depots Sep 26, 2021
@nielsmh
Copy link
Contributor

nielsmh commented Sep 26, 2021

So just to make sure I understand this: This is not about allowing "long" depots, or requiring that players build larger depots to contain bigger train or more trains, but simply letting multiple adjacent depots act as one single depot with multiple entryways?
Does this protect against "teleportation" exploits?

@J0anJosep
Copy link
Contributor Author

J0anJosep commented Sep 26, 2021

So just to make sure I understand this: This is not about allowing "long" depots, or requiring that players build larger depots to contain bigger train or more trains, but simply letting multiple adjacent depots act as one single depot with multiple entryways?

Yes

Does this protect against "teleportation" exploits?

No, it can be exploited by rearranging train chains. Currently depot size is limited by station_spread setting. So users could "teleport" wagons up to 64 tiles. I will look into it and think about adding a cost for moving vehicles from one tile to the other or moving the whole chain to an appropriate tile for making exploits more difficult.

@PeterN
Copy link
Member

PeterN commented Sep 27, 2021

Are non-contiguous depots prevented?

@2TallTyler
Copy link
Member

2TallTyler commented Sep 27, 2021

I like this a lot. Is it a (large) baby step toward #8480, or an alternative proposal?

@J0anJosep
Copy link
Contributor Author

J0anJosep commented Sep 27, 2021

Are non-contiguous depots prevented?

It should work the same way as building stations do. I (ab)use the same settings used for stations for "station spread" and "allow building distant stations not directly adjacent".
If "Allow to join stations not directly adjacent" is on, you can press Ctrl and join a new depot tile to another close depot. If the same setting is off, no non-contiguous depots can be built directly.

Whether it is necessary to create two new settings or rename the existing ones is something I haven't decided yet.

I like this a lot. Is it a (large) baby step toward #8480, or an alternative proposal?

It is the first part of #8480, but without including extended depots. This is no alternative proposal, but a first step.

I will try and fix the possible exploit and change the current draft state. Anyway, adding another setting for depot spread and setting it to "2" would fix it.

@glx22
Copy link
Contributor

glx22 commented Sep 27, 2021

I did a quick test, joining 2 depots of the same type but on different network is not a good idea :). Newly built train is stuck on the northern depot which is on the wrong network. But that's a user error I think.

@J0anJosep
Copy link
Contributor Author

I have added a setting for controlling depot spread instead of reusing the station spread setting.

About the teleportation exploit, it is very difficult to know in which tile a vehicle ends up after a rearrangement or replacement. Yet it is a possible exploit, it is difficult to apply and to get an advantage of. Anyway, setting a low value for max. depot spread should help with this and I do not want to add extra cost for moving vehicles.

Also, this last year I have had in mind modular airports while working in the "multi-tile depots" and the "extended depot" concept. This month I have finished reviewing how aircraft and hangars are integrated in this PR, so it doesn't make modular airports more difficult to be introduced (at least with my idea of how modular airports should work).

I have finished working on this PR. If it is acceptable to start reviewing it (even with the teleportation exploit not being fixed), I could remove the "Draft" state. Just let me know.

@2TallTyler
Copy link
Member

IMO, limiting depot spread is a perfectly valid way of preventing a possible teleportation aspect, since it's similar to how we control station teleportation.

assert(Depot::CanAllocateItem());
assert(this->GetNumHangars() > 0);
Station *st = Station::GetByTile(this->GetHangarTile(0));
this->hangar = new Depot(this->GetHangarTile(0));

Check warning

Code scanning / CodeQL

Resource not released in destructor Warning

Resource hangar is acquired by class Airport but not released in the destructor. It is released from RemoveHangar on line 755, so this function may need to be called from the destructor.
# Conflicts:
#	src/saveload/saveload.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants