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

Fix #7626: Allow building of drive-through stops over one-way/blocked roads owned by towns (instead of crashing). #7627

Merged
merged 1 commit into from Sep 6, 2019

Conversation

@ddm999
Copy link
Contributor

ddm999 commented Jun 26, 2019

Originally I thought about showing an error and preventing the stop from being built to prevent people from destroying a town's one-way system, but you can do that regardless by just removing the roads, so I don't think it really matters.
Makes more sense to just allow the stop to be placed.

…ads owned by towns (instead of crashing).
@LordAro
LordAro approved these changes Jul 6, 2019
@stormcone

This comment has been minimized.

Copy link
Contributor

stormcone commented Jul 12, 2019

Personally I better like @JGRennison's fix (JGRennison/OpenTTD-patches@2164eb4) in #7626. That is shorter and using the already available part of the CheckOwnership function.

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Jul 13, 2019

Agree with @stormcone, this fix looks weird.

@ddm999

This comment has been minimized.

Copy link
Contributor Author

ddm999 commented Jul 15, 2019

That fix prevents the building of drive-through station on one-way road, mine instead allows it.

With that fix, you can still just destroy the one-way road, then build a drive-through station in it's place.
This is why I decided on allowing the station to be built instead: by making the check for one-way road also check if it is town-owned.

Also with that fix, the error shown when attempting to place a station is the generic CheckOwnership one:

Can't build x ... owned by y (closest town name, in this instance)

which I don't think really makes sense to the player (doesn't specify the reason being that it's a one-way road).

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Sep 6, 2019

Okay this does look correct on a second check.

At first it looked like this fix would prevent you from building stops over your own one-way roads, but all it really does is circumvent the assert(owner != OWNER_TOWN) check in CheckOwnership(). The entire purpose CheckOwnership() is to confirm whether the current company is the owner and otherwise generate a relevant error, with the error generation really being its raison d'être.

@nielsmh nielsmh merged commit 2d9eb1c into OpenTTD:master Sep 6, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190626.1 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Sep 6, 2019

Also, the command has previously established that if the road was town owned, the player is allowed to build a road stop over it, so this is definitely the correct behaviour. It would be strange if you're allowed to build road stops over town roads, unless it's a one-way town road then you're not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.