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 #8055: Always allow building any available roadtypes, even if there are no road vehicles available #8063

Closed
wants to merge 4 commits into from

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Apr 5, 2020

No description provided.

@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Apr 5, 2020
@LordAro LordAro requested review from glx22 and nielsmh April 5, 2020 18:20
@LordAro
Copy link
Member Author

LordAro commented Apr 5, 2020

I've been doing some thinking on how I think it should be:

  • Road - build whenever there's an available roadtype, but disable stations when no vehicles are available
  • Ship - Same as road - canals and other water infrastructure should be able to be built, but not docks
  • Train - Only if there are trains available (makes no sense to me to be able to build rails if you don't have any trains to put them on, so keep as is)
  • Aircraft - Only if there are aircraft available (same as trains)

Yay/Nay?

@glx22
Copy link
Contributor

glx22 commented Apr 5, 2020

I'm totally OK with your reasoning.

@nielsmh
Copy link
Contributor

nielsmh commented Apr 5, 2020

Aircraft: Should heliport building be available when there are not VTOL aircraft available? Regular airport building be available if there are no airplanes (but helicopters/blimps exist)?

@LordAro
Copy link
Member Author

LordAro commented Apr 5, 2020

That's part of the aircraft build window, not the main toolbar :)
But for consistency with road/railtypes, i guess heliports should be disabled if there are no helicopters available (though airports should be buildable if only helicopters are available, as they have hangars)

@LordAro LordAro removed the backport requested This PR should be backport to current release (RC / stable) label Apr 7, 2020
@FLHerne
Copy link
Contributor

FLHerne commented Apr 7, 2020

I don't understand why this is desirable, nor why it's labeled as a "Fix".

The current behaviour is a feature that someone deliberately added. It's helpful to avoid players mistakenly building infrastructure they can't use. That situation exists in many games with commonly-chosen settings, and the behaviour is consistent between all infrastructure type.

This change makes the behaviour inconsistent between infrastructure types, and opens up inexperienced or just distracted players to frustrating mistakes.

It's of use only in the case where:

  1. No road/sea vehicles are available, which is fairly unusual.
  2. Someone [for reasons unexplained, either here or in the linked report] wants to build useless infrastructure.
  3. They haven't used the setting removed in this series, which allows exactly that behaviour as a non-default option.

(skim-reading the patch, it looks like the option might be non-functional for roadtypes? If so, that would be a real fix)

Why effectively flip the default behaviour, and then remove the option, in favour of a niche use-case rather than consistency and newbie-friendliness?

@Eddi-z
Copy link
Contributor

Eddi-z commented Apr 7, 2020

Roads are special, because you might want to influence the way towns grow, even if you don't have any road vehicles to build

As for VTOL, that's kinda the same as railtypes like monorail/maglev?

@andythenorth
Copy link
Contributor

I am +/-0 on whether to make the change.

BUT

We ban building currently because it's fricking annoying to build a route then discover there are no vehicles available.

It's an easy mistake to make if the route building tools aren't disabled, and we have not found any other way to assist players to avoid the mistake.

@nielsmh
Copy link
Contributor

nielsmh commented Apr 8, 2020

We ban building currently because it's fricking annoying to build a route then discover there are no vehicles available.

It's an easy mistake to make if the route building tools aren't disabled, and we have not found any other way to assist players to avoid the mistake.

Idea on warnings players that while they can build the infrastructure, no vehicles are available: A warning bar that shows up above (or below) the build toolbar. I also like the idea of disabling the station and depot construction tools.

image

Edit: This can also be extended to ships, so companies can build water infrastructure at all times.

Comment on lines 3045 to 3049
[SDTC_BOOL]
var = gui.disable_unsuitable_building
flags = SLF_NOT_IN_SAVE | SLF_NO_NETWORK_SYNC
def = true
str = STR_CONFIG_SETTING_DISABLE_UNSUITABLE_BUILDING
strhelp = STR_CONFIG_SETTING_DISABLE_UNSUITABLE_BUILDING_HELPTEXT
proc = RedrawScreen
flags = SLF_NOT_IN_SAVE | SLF_NO_NETWORK_SYNC | SLF_NOT_IN_CONFIG
def = false
cat = SC_EXPERT
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the entire block needs to be deleted to fix the compile error. Since it's not in save and not network synced it shouldn't be a problem to remove it from the settings order.

@Eddi-z
Copy link
Contributor

Eddi-z commented Apr 8, 2020

Idea on warnings players that while they can build the infrastructure, no vehicles are available: A warning bar that shows up above (or below) the build toolbar. I also like the idea of disabling the station and depot construction tools.

How about one of those red warning dialogs the first time you open the construction menu? It would leave more space to explain what's going on.

@TrueBrain
Copy link
Member

Please leave other languages to eints; could you remove that from this PR? Tnx :)

@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: extra large This Pull Request is very large; it properly needs to be split up before it can be properly reviewed labels Dec 14, 2020
@andythenorth
Copy link
Contributor

Divide the roadtype list into 2, with a horizontal separator?

  • roads with vehicles available
  • roads without vehicles available

??

@LordAro
Copy link
Member Author

LordAro commented Dec 25, 2020

I like the idea of a separated drop-down list, but I've no idea how we'd label the 2 lists

@TrueBrain Other language modifications, sure, but removals too? Leaving them in would cause compile warnings until the next eints run, which I'm not a particular fan of

@andythenorth
Copy link
Contributor

We could count the number of vehicles available for each type? And show that in a badge after the name

e.g. "Trails (10 vehicles available)", "Yellow Brick Road (0 vehicles available)".

I'm not convinced by that at all, but it's an idea we can kick around a bit?

@James103
Copy link
Contributor

Or maybe change the color of the text of the road or rail type when the road/track type is available, but no vehicles are available for that type?

@TrueBrain
Copy link
Member

TrueBrain commented Dec 25, 2020

@TrueBrain Other language modifications, sure, but removals too? Leaving them in would cause compile warnings until the next eints run, which I'm not a particular fan of

Wrote it on IRC, but might as well here too :D

This is what we have been doing a lot lately (after frosch123 pointed out we shouldn't be doing it by hand), and it works fine. Sure, till eints runs you will get warnings, but you can run eints manually if you want to (like I did today). Either way, eints does its thing once a day, before the nightly, so worst case you are left with warnings for a whopping 24 hours :)
Removals especially shouldn't be done manually, as it is very hard for a reviewer to spot if you made a mistake, and if you by accident remove the wrong string from other languages, that does more harm. It really is better to leave it to a tool that is designed to take care of such jobs :)

So especially removals of other languages should be left to eints; not to humans :)

As example: #8426 <- only did the renaming, not the removals. This was done by eints here: 7b515fa

PS: manually runs can be done by devs here: https://github.com/OpenTTD/workflows/actions?query=workflow%3A%22Eints+to+Git%22

@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
@@ -1756,7 +1756,6 @@ bool CanBuildVehicleInfrastructure(VehicleType type, byte subtype)
assert(IsCompanyBuildableVehicleType(type));

if (!Company::IsValidID(_local_company)) return false;
if (!_settings_client.gui.disable_unsuitable_building) return true;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. I think "removing disable_unsuitable_building" should mean "set it to false".
So this line and everything below should be replaced with "return true".

@TrueBrain
Copy link
Member

With #8521 I took a similar but different approach to this PR (in fact, I "stole" some commits :D).

I fully agree with the issue that just because max-road-vehicle is zero, it doesn't mean I shouldn't be able to build roads. And to say: then you have to change a setting, feels a bit weird to me.

So what I basically did is: always enable the toolbar buttons, but disable the build depot/station etc (similar as in last image here). Next, I changed the tooltip to, when hover over, show why they are disabled. I think that informs the player sufficiently what the situation is, while still allowing building roads and canals before you have vehicles available.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: extra large This Pull Request is very large; it properly needs to be split up before it can be properly reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants