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: Reload NewGRF on SIGUSR1 signal #9004

Closed
wants to merge 1 commit into from

Conversation

rexxars
Copy link

@rexxars rexxars commented Apr 10, 2021

Motivation

Developing and tweaking NewGRFs can be quite tedious, especially the edit => build => reload cycle. I've improved on this somewhat by having a watcher on graphics, languages and NML files which rebuilds on changes, but I still have to switch to OpenTTD, enter the console and type the newgrf reload command. It would be awesome to avoid this last part in a way that allowed OpenTTD to stay open and not even require refocusing the window to see my changes.

Description

This PR implements a SIGUSR1 signal handler (open to changing this, SIGHUP or SIGINFO might be good alternatives), which calls the same ReloadNewGRFData() method that the console command calls, when receiving the signal.

This enables NewGRF tooling (like my setup) to trigger the signal after a build.

Limitations

As far as I know there's no equivalent signal handling for windows, so this is currently only available for unix-based systems.

I am totally unfamiliar with the OpenTTD codebase and a novice at C++, so I'm unsure if this is the best place to put the signal handling and if the approach is acceptable, but figured I'd give it a try.

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

@nielsmh
Copy link
Contributor

nielsmh commented Apr 10, 2021

The place you've put the signal handler probably won't work, or at least only by chance. It only sets the handlers during savegame load, they are reset to default after the save-load has finished.
The more appropriate place would be in openttd.cpp during or after the initial NewGRF scanning.

On Windows there are several mechanisms that would work. The closest equivalent would be posting a window message, but there could also be writing to a named pipe or raising an event in the global namespace, or any other kind of kernel synchronization object. Problem is just that there aren't standard commandline tools to trigger any of those, unless you added a commandline option to OpenTTD itself that would cause the appropriate trigger.

@PeterN
Copy link
Member

PeterN commented Apr 10, 2021

Might make sense to guard the signal handler by checking if newgrf developer tools are enabled.

@JGRennison
Copy link
Contributor

Could the admin port be a reasonably portable trigger mechanism?

@rexxars
Copy link
Author

rexxars commented Apr 10, 2021

The more appropriate place would be in openttd.cpp during or after the initial NewGRF scanning.

Thanks for the pointer! I've moved the code into openttd.cpp as suggested, let me know if it still needs to be moved.

On Windows there are several mechanisms that would work. The closest equivalent would be posting a window message, but there could also be writing to a named pipe or raising an event in the global namespace, or any other kind of kernel synchronization object. Problem is just that there aren't standard commandline tools to trigger any of those, unless you added a commandline option to OpenTTD itself that would cause the appropriate trigger.

That seems like it might be a lot of work. Do you feel we need to have feature parity for this on windows for this to be accepted?

Might make sense to guard the signal handler by checking if newgrf developer tools are enabled.

Good point! I've wrapped the registration of the signal handler in a conditional now.

Could the admin port be a reasonably portable trigger mechanism?

I'm not familiar with the admin port - but from what I can tell it is not enabled unless you run a multiplayer game. In the case of NewGRF development, I think either singleplayer or the scenario editor is the most common way to develop.

For instance, I am creating a set of NewGRF objects that is only placable in the scenario editor, and to test both the "purchase" (object placement) dialog as well as how they appear ingame, having the functionality available in the scenario editor would be great.

@nielsmh
Copy link
Contributor

nielsmh commented Apr 10, 2021

Yeah the code placement looks fine now. However I'm not sure you should be reloading directly in the signal handler, as that can interrupt absolutely anything going on. In particular, there's a pretty big risk the main loop is running and is in some code that could be sensitive to NewGRF data changing underneath it. It would be better to set a flag (std::atomic<bool>) which is checked somewhere in the main loop. That would also make the mechanism easier to extend to Windows and keep behaviour the same.

@rexxars
Copy link
Author

rexxars commented Apr 10, 2021

However I'm not sure you should be reloading directly in the signal handler, as that can interrupt absolutely anything going on. In particular, there's a pretty big risk the main loop is running and is in some code that could be sensitive to NewGRF data changing underneath it.

Wouldn't that be the case for the console command too, then?

@nielsmh
Copy link
Contributor

nielsmh commented Apr 10, 2021

No, the console command is handled during the input processing, which is at a well-known time. Specifically, not while the main loop is running the simulation.

@rexxars
Copy link
Author

rexxars commented Apr 10, 2021

No, the console command is handled during the input processing, which is at a well-known time. Specifically, not while the main loop is running the simulation.

Ah. Thanks for explaining :)

I've updated to use an atomic bool which is checked during the main loop as you suggested.

nielsmh
nielsmh previously approved these changes Apr 11, 2021
Copy link
Contributor

@nielsmh nielsmh left a comment

Choose a reason for hiding this comment

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

Not tested myself, but looks fine now.
We can add a method for Windows users later if there's any demand.

@@ -498,6 +515,12 @@ struct AfterNewGRFScan : NewGRFScanCallback {
NetworkClientConnectGame(NetworkAddress(network_conn, rport), join_as, join_server_password, join_company_password);
}

#if defined(UNIX)
if (_settings_client.gui.newgrf_developer_tools) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this test should be in the handler

Copy link
Contributor

Choose a reason for hiding this comment

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

True. As written you need to quit and restart the game after enabling newgrf_developer_tools for the signal feature to be available.

@TrueBrain
Copy link
Member

My worry with this changes is a few-fold:

  1. it is likely we forget this was ever added, and gives for some surprises in a few years
  2. I am not sure anyone else will use this in its current form (Linux only, only useful for NewGRF authors that are watching their files. A very single specific use-case)
  3. it is specific for 1 scenario, but I can think of a few others that might want to automate things like this. For example, AI writers, reloading maps constantly, etc.

Basically, I see this as misuse of a signal for 1 specific itch. I wonder if we cannot do better and make it a bit more common, so it is more likely more people can make use of it?

@nielsmh nielsmh dismissed their stale review April 11, 2021 20:14

I've been convinced this needs more discussion.

@LordAro LordAro added candidate: needs considering This Pull Request needs more opinions candidate: probably not This Pull Request will most likely be closed soon and removed candidate: needs considering This Pull Request needs more opinions labels Apr 16, 2021
@rubidium42
Copy link
Contributor

Basically, I see this as misuse of a signal for 1 specific itch. I wonder if we cannot do better and make it a bit more common, so it is more likely more people can make use of it?

Maybe call a script, just like "autoexec.scr" but on a signal ("on_signal.scr")? Then the depending on their itch the user can change the behavior. Having said that, the point of it not working on all platforms remains, and "forgetting" it does exists remains too (I forgot/didn't even know "autoexec.scr" existed).

The admin port at least requires less infrastructure in OpenTTD, although it might need to be configurable to just always run regardless of it being a server. But alas, that is yet another setting but opening an admin port for everyone playing might not be the best thing to do either. Then it's just the matter of writing a simple admin port send command tool which could be used in a similar manner as signal on Unix, but with better cross platform compatibility and fewer problems with injecting the command at the wrong moment in he running of the application.

@rexxars
Copy link
Author

rexxars commented Apr 27, 2021

Closing this as people are not onboard, and there doesn't seem to be a clear path forward. Admin port doesn't work in scenario editor, needs opening a port etc. Will build a local copy for the change instead.

@rexxars rexxars closed this Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: probably not This Pull Request will most likely be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants