Feature: option to auto remove signals when in the way during rail co… #8274

Kuhnovic · 2020-07-15T12:00:38Z

Discussion on forum https://www.tt-forums.net/viewtopic.php?f=33&t=87295

A setting (default = off) to automatically remove signals when they are in the way. This is especially convenient when reworking station entries and branching off of mainlines. It also makes sure your depots always get attached (if you are close to a piece of track), instead of a signal in the way silently preventing the entry/exit track from being generated.

The setting can be found under Company settings. The setting is not shared during network games, so everyone can have their own preference.

Kuhnovic · 2020-07-15T12:56:59Z

I am aware of the failing check. I'm trying to fix it now, but I'm new to Git and it's rather overwhelming. Can I somehow modify my original commit, or should I just add a second one?

LordAro · 2020-07-15T13:07:01Z

You can indeed modify your commit history - https://docs.github.com/en/github/using-git/about-git-rebase

Kuhnovic · 2020-07-27T09:52:43Z

Not sure if this was clear, but I'm done fixing, so the reviewing can commence 😉 . I don't mean to nag, I'm just excited as this is my first contribution, forgive me 😄

I tested this feature during several long multiplayer sessions with friends (windows + linux, with and without FIRS 3 newGRF) and it held up great, no issues found so far.

src/lang/english.txt

M3Henry · 2020-07-29T16:36:57Z

I like the idea, buildings that are in the way could also be covered by this

A confirmation dialogue that pops up might avoid mistakes could be a third configurable option

never
ask
always

techgeeknz · 2020-07-29T20:47:19Z

I don't mean to nag, I'm just excited as this is my first contribution, forgive me 😄

Awesome first commit, @Kuhnovic. Welcome to the club 🥇.

Kuhnovic · 2020-07-30T07:45:11Z

I like the idea, buildings that are in the way could also be covered by this

A confirmation dialogue that pops up might avoid mistakes could be a third configurable option

never

ask

always

Buildings aren't quite a nuisance (at least IMO). Also, if you accidentally delete them, you can't put them back, so I don't think it makes as much sense as signals.

I was actually considering this never / ask / always option, but for now I just went with on or off. I should try this out sometime! For now however, I want to avoid feature creep ;)

ldpl · 2020-07-30T09:17:31Z

I kinda feel like "ask" is pretty much the same as "never". Though it may be useful for some players (e.g. android) it doesn't match any other tool and will probably be a bit cumbersome to implement.

Kuhnovic · 2020-07-30T15:55:12Z

I kinda feel like "ask" is pretty much the same as "never". Though it may be useful for some players (e.g. android) it doesn't match any other tool and will probably be a bit cumbersome to implement.

I quickly put something together. It doesn't actually respond do the Yes or No buttons, it removes it anyway, but that's not the point. I wanted to see how the interaction with the dialog felt. IMHO it's even more annoying than having to remove the signal by hand, since that's something that can be done reasonably quickly using hotkeys.

It also became clear how much extra logic it would require to get it to work. You'd first have to check the entire stretch of rail you want to build for signals that are in the way, show the dialog if applicable, and re-run the command with the right settings to remove the rail (or not, but then you probably should not show an error either). It can be done, but it makes things pretty complex, for very little gain if you ask me.

Kuhnovic · 2020-10-02T14:06:13Z

So is there any follow-up I should be doing? It's been a few months now and I haven't heard anything, and the PR is still open. If there's action for me to take, please let me know :)

TrueBrain

Sorry it took us a while to get back to this. There are some minor coding-style issues I would like to have fixed, and this needs a rebase. If you are still up for it, I would love to have this PR merged.

I have not play-tested this yet, I will do that after you fixed and rebased this PR :)

Tnx!

src/rail_cmd.cpp

TrueBrain · 2021-01-05T11:28:16Z

src/rail_cmd.cpp

+					}
+					else
+					{
+						return_cmd_error(STR_ERROR_MUST_REMOVE_SIGNALS_FIRST);


If you flip the if() around (so if (!autoRemoveSignals) {), you can return, and don't need the else. This makes the code more readable I think.

src/rail_cmd.cpp

TrueBrain · 2021-01-05T11:33:26Z

src/rail_cmd.cpp

 * @param text unused
 * @return the cost of this operation or an error
 */
 CommandCost CmdBuildSingleRail(TileIndex tile, DoCommandFlag flags, uint32 p1, uint32 p2, const char *text)
 {
 	RailType railtype = Extract<RailType, 0, 6>(p1);
 	Track track = Extract<Track, 0, 3>(p2);
+	bool autoRemoveSignals = HasBit(p2, 3);


please use: auto_remove_signals

src/settings_type.h

TrueBrain · 2021-01-05T11:40:57Z

src/rail_cmd.cpp

@@ -893,7 +915,7 @@ static CommandCost CmdRailTrackHelper(TileIndex tile, DoCommandFlag flags, uint3
 	bool had_success = false;
 	CommandCost last_error = CMD_ERROR;
 	for (;;) {
-		CommandCost ret = DoCommand(tile, remove ? 0 : railtype, TrackdirToTrack(trackdir), flags, remove ? CMD_REMOVE_SINGLE_RAIL : CMD_BUILD_SINGLE_RAIL);
+		CommandCost ret = DoCommand(tile, remove ? 0 : railtype, TrackdirToTrack(trackdir) | auto_remove_signals << 3, flags, remove ? CMD_REMOVE_SINGLE_RAIL : CMD_BUILD_SINGLE_RAIL);


please change auto_remove_signals << 3 to (auto_remove_signals << 3). It is more explicit, and avoids issues for future-us :) (goes for all instances of << in your patch).

src/table/settings.ini

kabal2020 · 2021-01-05T19:24:19Z

Couldn't this lead to train crashes if set to "always" by default?

Drag a new bit of track a bit too far/misclick (especially easy on a small screen on Android) and delete a signal on a live track with trains on it?

TrueBrain · 2021-01-05T19:25:44Z

Couldn't this lead to train crashes if set to "always" by default?

Drag a new bit of track a bit too far/misclick (especially easy on a small screen on Android) and delete a signal on a live track with trains on it?

To quote the tooltip:
Automatically remove signals during rail construction if the signals are in the way. Note that this can potentially lead to train crashes.

kabal2020 · 2021-01-05T19:38:36Z

To quote the tooltip:
Automatically remove signals during rail construction if the signals are in the way. Note that this can potentially lead to train crashes.

Ah right, see it now, hurrah - sorry new to this (came here from your Twitch stream - hope I'm not poking my nose in the wrong places!)

TrueBrain · 2021-01-05T19:40:13Z

To quote the tooltip:
Automatically remove signals during rail construction if the signals are in the way. Note that this can potentially lead to train crashes.

Ah right, see it now, hurrah - sorry new to this (came here from your Twitch stream - hope I'm not poking my nose in the wrong places!)

Never! Ask anything you like, we will tell you off when you cross any line ;) Tnx, and welcome :)

…nstruction

Kuhnovic · 2021-01-06T10:06:04Z

Just out of curiosity, how is that playtesting mechanism set up? It looks really neat!

And as a Dutchman myself I gotta love the name DorpsGek, "Village Idiot". Nice.

TrueBrain · 2021-01-06T11:50:08Z

Just out of curiosity, how is that playtesting mechanism set up? It looks really neat!

And as a Dutchman myself I gotta love the name DorpsGek, "Village Idiot". Nice.

Tnx! It took some effort, but the result is amazing :D I should write a blog-post about it, I guess .. or a forum-post ...

In a very short version:

Emscripten allows us to build OpenTTD as a WASM variant, which can run in your browser
GitHub Actions allows us to build a new version when people push new code in their PR
And there is some glue in between to host the files on a place you can visit, with GitHub Deployments.

If you want to know more, drop by on IRC or Discord and we can chat :)

Welcome to a new world :D

TrueBrain

Tested various of situations; does what it claims that it does :P

Nice work, thank you for the Pull Request!

Kuhnovic force-pushed the autoremovesignals branch 2 times, most recently from 1b4ffed to 1024492 Compare July 16, 2020 11:19

Eddi-z reviewed Jul 27, 2020

View reviewed changes

src/lang/english.txt Outdated Show resolved Hide resolved

Kuhnovic force-pushed the autoremovesignals branch 2 times, most recently from 61edc43 to 06481ea Compare July 27, 2020 14:51

TrueBrain added candidate: probably not size: large labels Dec 14, 2020

TrueBrain added candidate: yes work: minor details and removed candidate: probably not labels Jan 5, 2021

TrueBrain requested changes Jan 5, 2021

View reviewed changes

JGRennison reviewed Jan 5, 2021

View reviewed changes

src/table/settings.ini Outdated Show resolved Hide resolved

TrueBrain added this to the 1.11.0 milestone Jan 5, 2021

Feature: option to auto remove signals when in the way during rail co…

Loading
Loading status checks…

a503940

…nstruction

Kuhnovic force-pushed the autoremovesignals branch from 06481ea to a503940 Compare January 5, 2021 20:31

TrueBrain added the preview label Jan 5, 2021

DorpsGek temporarily deployed to preview-pr-8274 January 5, 2021 20:32 Inactive

TrueBrain approved these changes Jan 6, 2021

View reviewed changes

TrueBrain removed the work: minor details label Jan 7, 2021

TrueBrain merged commit a3a7928 into OpenTTD:master Jan 7, 2021

Kuhnovic deleted the autoremovesignals branch January 7, 2021 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: option to auto remove signals when in the way during rail co… #8274

Feature: option to auto remove signals when in the way during rail co… #8274

Kuhnovic commented Jul 15, 2020 •

edited

Loading

Kuhnovic commented Jul 15, 2020

LordAro commented Jul 15, 2020

Kuhnovic commented Jul 27, 2020 •

edited

Loading

M3Henry commented Jul 29, 2020

techgeeknz commented Jul 29, 2020

Kuhnovic commented Jul 30, 2020 •

edited

Loading

ldpl commented Jul 30, 2020

Kuhnovic commented Jul 30, 2020 •

edited

Loading

Kuhnovic commented Oct 2, 2020

TrueBrain left a comment

TrueBrain Jan 5, 2021

TrueBrain Jan 5, 2021

TrueBrain Jan 5, 2021

kabal2020 commented Jan 5, 2021

TrueBrain commented Jan 5, 2021

kabal2020 commented Jan 5, 2021

TrueBrain commented Jan 5, 2021

Kuhnovic commented Jan 6, 2021

TrueBrain commented Jan 6, 2021

TrueBrain left a comment

Feature: option to auto remove signals when in the way during rail co… #8274

Feature: option to auto remove signals when in the way during rail co… #8274

Conversation

Kuhnovic commented Jul 15, 2020 • edited Loading

Kuhnovic commented Jul 15, 2020

LordAro commented Jul 15, 2020

Kuhnovic commented Jul 27, 2020 • edited Loading

M3Henry commented Jul 29, 2020

techgeeknz commented Jul 29, 2020

Kuhnovic commented Jul 30, 2020 • edited Loading

ldpl commented Jul 30, 2020

Kuhnovic commented Jul 30, 2020 • edited Loading

Kuhnovic commented Oct 2, 2020

TrueBrain left a comment

Choose a reason for hiding this comment

TrueBrain Jan 5, 2021

Choose a reason for hiding this comment

TrueBrain Jan 5, 2021

Choose a reason for hiding this comment

TrueBrain Jan 5, 2021

Choose a reason for hiding this comment

kabal2020 commented Jan 5, 2021

TrueBrain commented Jan 5, 2021

kabal2020 commented Jan 5, 2021

TrueBrain commented Jan 5, 2021

Kuhnovic commented Jan 6, 2021

TrueBrain commented Jan 6, 2021

TrueBrain left a comment

Choose a reason for hiding this comment

Kuhnovic commented Jul 15, 2020 •

edited

Loading

Kuhnovic commented Jul 27, 2020 •

edited

Loading

Kuhnovic commented Jul 30, 2020 •

edited

Loading

Kuhnovic commented Jul 30, 2020 •

edited

Loading