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: allow SE and GS to define spacing between road layouts of towns #8442

Closed
wants to merge 1 commit into from

Conversation

TrueBrain
Copy link
Member

Motivation / Problem

#7745 introduces a setting for more spacing between roads in towns. Although it visually looks nice, it also introduces a bunch of problems and questions. As can be seen in that PR, people immediately point out that with values > 3 things get a bit wonky, and as such, follow-up PRs are expected. But where do you draw the line between constantly evolving Town algorithms hard-coded in C++, and when are you going to delegate this to NewGRF/GS.

Especially the introduction of yet-another-setting can only lead to more bug-reports, which is counter-productive from our point of view.

As the basis of the PR was solid, and improves the road algorithm drastic (by doing away with all all the variants already existing), I wanted to cherry-pick that work out. While doing so, I found out that allowing the SE and GS build towns with higher spacing is exactly in line with the ideology set out by the project goals.

After this PR, more work is needed for GS to get more control over towns, but I would consider this a start.

This PR is a counter-proposal against #7745.

Description

  • Rework the code to store a layout (natural or grid) and a spacing (1-3). This has no functional difference.
  • Allow GS (and in some extend, AIs) to create towns where they can define the layout and spacing (1-6).
  • Allow SE to create towns with layout and spacing (1-6).

TODO

  • The GUI does not yet allow to set the spacing in the SE.
  • I am not too happy about the code, mainly as the name TownLayoutSpacing and TownLayoutSetting look way to similar.
  • I am not all too happy how TLS_RANDOM works. The original code was rather strange here, so it needs some more thinking.

Limitations

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 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')

Co-authored-by: Johannes E. Krause <j.k@eclipso.de>
@frosch123
Copy link
Member

frosch123 commented Dec 27, 2020

What does this PR do?

I don't think hding settings improves the situation, or increases moddability.

@Eddi-z
Copy link
Contributor

Eddi-z commented Dec 27, 2020

Imho, as far as a modding interface goes, this goes not nearly far enough to bother.

@andythenorth
Copy link
Contributor

PR permits experiments, Rome wasn't built in a day etc.

@TrueBrain
Copy link
Member Author

Let me reword what I think what this PR does:

* It adds the same algorithms #7745.

No, most of 7745 reworks the code to be more readable (and have less exceptions). I like that part of the code, and that is what I ported over.

* It adds the same settings as #7745, but hides them from the user, and allows GS to set them.

It does not add any setting, and as such, it does not "hide" it from the user. But that feels like a bit of a tomato tomato discussion :)

* It does not give GS more control over different road placing.

It is a start at least. We have to start somewhere, don't we? ;)

I don't think hding settings improves the situation, or increases moddability.

Here we differ in opinion; I am not hiding any setting, as it was never any setting. If we go by this logic, we hide many many settings ;) I would rather call it: we take an opinion on what the base-game does, and GSes can overwrite that.

And again, this is just meant to show we could also do it this way, as a start to making towns a bit more moddable. But I can also just leave out that part, and only do the refactoring.

@andythenorth
Copy link
Contributor

andythenorth commented Dec 27, 2020

This goes off-topic from the PR (and I am happy to move it to a discussion) BUT

It took about 10 years for the industry (and cargo) spec to reach the current high level of maturity. 😄 🥰

We had the basic 3-in, 2-out, let's copy TTDP approach somewhere around 0.7x or 0.8x.

But we didn't have an established approach to cargo labels or classes.

We didn't have the industry and cargo flow charts.

We didn't have advanced action 2 sprite layouts.

We didn't have 64 cargos and 128 industries.

We didn't have 16-in, 16-out cargos.

We didn't have 256 permanent registers per industry.

We didn't have useful control over the text in the industry window.

Stuff takes time. And we have to make a few wrong moves. Which we will regret. But no progress without some risk of regret.

I post this mini-wall of text because I would like to see towns and the economy be more completely moddable.

That will take time.

But I think it would be good for the health of OpenTTD

  • both for players,
  • but also for contributors, by reducing issues and PRs about 'change X facet of towns' etc. These are tedious PRs to review. 😺

But I can only think of 2 routes to get there

  • somebody produces a near-complete spec (this has worked multiple times)
  • trial and error, with experiments featuring working code

@TrueBrain
Copy link
Member Author

Talked this over a bit; basically, although I think this is just a random step into making towns more configurable from GS, it is just that: a random step. We really should have a goal first, and a bit of a design, before we do such thing. So adding the GS stuff in here, is just not that useful.

And if I look at the other code changes, although I like it makes a lot of things simpler, it is just not worth the code changes now, without know where we want to move to (as it kinda assumes the "spacing" part would remain).

So I am going to close this PR for now, to keep it in my back pocket till someone makes a decent plan :)

@TrueBrain TrueBrain closed this Dec 27, 2020
@TrueBrain TrueBrain deleted the town_space branch January 17, 2021 15:09
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