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
Conversation
5049252
to
64f4c06
Compare
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. 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. |
Might make sense to guard the signal handler by checking if newgrf developer tools are enabled. |
Could the admin port be a reasonably portable trigger mechanism? |
64f4c06
to
b80d83c
Compare
Thanks for the pointer! I've moved the code into
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?
Good point! I've wrapped the registration of the signal handler in a conditional now.
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. |
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 ( |
Wouldn't that be the case for the console command too, then? |
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. |
b80d83c
to
0d7c420
Compare
Ah. Thanks for explaining :) I've updated to use an atomic bool which is checked during the main loop as you suggested. |
0d7c420
to
52515f6
Compare
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
My worry with this changes is a few-fold:
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? |
I've been convinced this needs more discussion.
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. |
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. |
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
orSIGINFO
might be good alternatives), which calls the sameReloadNewGRFData()
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.