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.

Sign up for GitHub

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

Jump to bottom

Fix #9521: Don't load at just removed docks that were part of a multi-dock station #9524

Merged
merged 1 commit into from Sep 18, 2021
Merged

Fix #9521: Don't load at just removed docks that were part of a multi-dock station #9524

merged 1 commit into from Sep 18, 2021

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Aug 30, 2021 •
edited

Motivation / Problem

Fixes #9521

Given the scenario with a station with 2 (or more) docks (with 2 docking tiles), if the ship is loading/unloading at one of the docks, and that dock is demolished, the ship still continues to load/unload, because the station facility FACIL_DOCK still exists.
It should instead, make the ship go to the other dock of that same station.

Description

Problem is solved by also checking if the docking tile where the ship is positioned at is still valid and belong to the station entity.
Savegame:
multidock 2.zip
In this savegame, ship 1 and ship 3 are unloading at Würzbrücken Docks which consist of 2 docks with 2 docking tiles.
Würzbrucken Valley is another dock that shares the same docking tile as that where ship 3 is located.
Tests:
1 - Removing Würzbrücken Valley dock should not affect ship 3 unloading at the other dock.
2 - Removing one of the Würzbrücken Docks should make the ship that it's at the respective docking tile to leave station and head to the other dock nearby that still belongs to the same station.

Limitations

  • There's a bit of code repetition.
  • I am always assuming tile3 is a docking tile and is retrieved in that manner. Maybe there is a better way to retrieve which tiles are the docking tiles.

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? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (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? (label 'needs review: NewGRF')

@TrueBrain TrueBrain added this to the 12.0 milestone Sep 14, 2021
@TrueBrain
Copy link
Member

TrueBrain commented Sep 18, 2021 •
edited

Although you seem to have traced the place where things go wrong, your solution is both hard to follow and possibly prune for errors. If the logic gets that complicated, it just becomes tricky.

The main issue I have with your code, is that tile3 is in, what, 90% of the cases false. Only if two docks of the same stations have overlap, it can yield true. In other words, the for (Ship..) is executed in most cases. So to remove complexity, we can basically just remove that if-statement.

Next, (!a || !b) is the same as !(a && b), which in this case is a lot more readable.

Also, a station without a dock is similar to the docking tile no longer belonging to the station. To reduce complexity, I suggest we simply remove the "does station have dock" check in favour of "does this docking-tile belong to the station". One implies the other, and it removes yet-another-check.

Lastly, the comment in front of the if was now heavily out-of-date. Fixing that comment would be really difficult. So instead, I suggest something like the following:

		for (Ship *s : Ship::Iterate()) {
			/* Find all ships going to our dock. */
			if (s->current_order.GetDestination() != st->index) {
				continue;
			}

			/* Find ships that are marked as "loading" but are no longer on a
			 * docking tile. Force them to leave the station (as they were loading
			 * on the removed dock). */
			if (s->current_order.IsType(OT_LOADING) && !(IsDockingTile(s->tile) && IsShipDestinationTile(s->tile, st->index))) {
				s->LeaveStation();
			}

			/* If we no longer have a dock, mark the order as invalid and send
			 * the ship to the next order (or, if there is none, make it
			 * wander the world). */
			if (s->current_order.IsType(OT_GOTO_STATION) && !(st->facilities & FACIL_DOCK)) {
				s->SetDestTile(s->GetOrderStationLocation(st->index));
			}
		}

This still fixes the problem (as far as I could tell), removes the need for tile3, and writes a story over multiple comments. Each statement is also easier to follow:

  • Does this ship go to this station? No? Skip.
  • Is it currently loading on a tile that doesn't belong to the station anymore? Leave the station.
  • Are we going to the station, but there are no docks anymore? Skip to next order.

@TrueBrain TrueBrain merged commit 18247bb into OpenTTD:master Sep 18, 2021
@SamuXarick SamuXarick deleted the multidock-loading/unloading branch December 5, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TrueBrain TrueBrain TrueBrain approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
12.0
2 participants
@SamuXarick @TrueBrain