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: option to auto remove signals when in the way during rail co… #8274

Merged
merged 1 commit into from Jan 7, 2021

Conversation

@Kuhnovic
Copy link
Contributor

@Kuhnovic Kuhnovic commented Jul 15, 2020

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.

Quick demo

@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jul 15, 2020

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
Copy link
Member

@LordAro LordAro commented Jul 15, 2020

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

@Kuhnovic Kuhnovic force-pushed the Kuhnovic:autoremovesignals branch 2 times, most recently from 1b4ffed to 1024492 Jul 15, 2020
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jul 27, 2020

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 Outdated Show resolved Hide resolved
@Kuhnovic Kuhnovic force-pushed the Kuhnovic:autoremovesignals branch 2 times, most recently from 61edc43 to 06481ea Jul 27, 2020
@M3Henry
Copy link
Contributor

@M3Henry M3Henry commented Jul 29, 2020

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
Copy link
Contributor

@techgeeknz techgeeknz commented Jul 29, 2020

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
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jul 30, 2020

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
Copy link
Contributor

@ldpl ldpl commented Jul 30, 2020

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
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jul 30, 2020

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.

YesNo

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
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Oct 2, 2020

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

Copy link
Member

@TrueBrain TrueBrain left a comment

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 Outdated Show resolved Hide resolved
src/rail_cmd.cpp Outdated Show resolved Hide resolved
}
else
{
return_cmd_error(STR_ERROR_MUST_REMOVE_SIGNALS_FIRST);

This comment has been minimized.

@TrueBrain

TrueBrain Jan 5, 2021
Member

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 Outdated Show resolved Hide resolved
* @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);

This comment has been minimized.

@TrueBrain

TrueBrain Jan 5, 2021
Member

please use: auto_remove_signals

src/settings_type.h Outdated Show resolved Hide resolved
@@ -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);

This comment has been minimized.

@TrueBrain

TrueBrain Jan 5, 2021
Member

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 Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
@kabal2020
Copy link

@kabal2020 kabal2020 commented Jan 5, 2021

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
Copy link
Member

@TrueBrain TrueBrain commented Jan 5, 2021

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
Copy link

@kabal2020 kabal2020 commented Jan 5, 2021

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
Copy link
Member

@TrueBrain TrueBrain commented Jan 5, 2021

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

@Kuhnovic Kuhnovic force-pushed the Kuhnovic:autoremovesignals branch from 06481ea to a503940 Jan 5, 2021
@Kuhnovic
Copy link
Contributor Author

@Kuhnovic Kuhnovic commented Jan 6, 2021

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
Copy link
Member

@TrueBrain TrueBrain commented Jan 6, 2021

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

Copy link
Member

@TrueBrain TrueBrain left a comment

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

Nice work, thank you for the Pull Request!

@TrueBrain TrueBrain merged commit a3a7928 into OpenTTD:master Jan 7, 2021
9 checks passed
9 checks passed
Emscripten
Details
Commit checker
Details
Check for preview label Check for preview label
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (x86) Windows (x86)
Details
Windows (x64) Windows (x64)
Details
@Kuhnovic Kuhnovic deleted the Kuhnovic:autoremovesignals branch Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet