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

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 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 autoremovesignals branch 2 times, most recently from 1b4ffed to 1024492 Compare July 16, 2020 11:19
@Kuhnovic
Copy link
Contributor Author

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 autoremovesignals branch 2 times, most recently from 61edc43 to 06481ea Compare July 27, 2020 14:51
@M3Henry
Copy link
Contributor

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

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

@TrueBrain TrueBrain added candidate: probably not This Pull Request will most likely be closed soon size: large This Pull Request is large in size; review will take a while labels Dec 14, 2020
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged work: minor details This Pull Request has some minor details left to do and removed candidate: probably not This Pull Request will most likely be closed soon labels Jan 5, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

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
src/rail_cmd.cpp Outdated
}
else
{
return_cmd_error(STR_ERROR_MUST_REMOVE_SIGNALS_FIRST);
Copy link
Member

Choose a reason for hiding this comment

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

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
src/rail_cmd.cpp Outdated
* @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);
Copy link
Member

Choose a reason for hiding this comment

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

please use: auto_remove_signals

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

Choose a reason for hiding this comment

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

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

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

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

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

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

@TrueBrain TrueBrain added the preview This PR is receiving preview builds label Jan 5, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8274 January 5, 2021 20:32 Inactive
@Kuhnovic
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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

Nice work, thank you for the Pull Request!

@TrueBrain TrueBrain removed the work: minor details This Pull Request has some minor details left to do label Jan 7, 2021
@TrueBrain TrueBrain merged commit a3a7928 into OpenTTD:master Jan 7, 2021
@Kuhnovic Kuhnovic deleted the autoremovesignals branch January 7, 2021 10:20
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 preview This PR is receiving preview builds size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet