-
-
Notifications
You must be signed in to change notification settings - Fork 929
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
Add #2155: newheightmapgame command #7286
Conversation
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.
The commit message also doesn't follow style. Should go in the direction of
Add #2155: newheightmapgame command
newheightmapgame
command (addresses #2155)
newheightmapgame
command (addresses #2155)e61d48b
to
5733a73
Compare
the name sounds a bit clumsy, maybe |
There is a |
16cf7bf
to
81b504d
Compare
dc7a1cd
to
3364161
Compare
Added a new feature commit to allow CLI loading of heightmaps by filename/name/title |
Made some suggestions in Berbe#1 |
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 StartNewHeightMapGameWithoutGUI() should return a bool, then you can use it in VideoDriver_Dedicated::MainLoop() to do a check similar to the savegame load check
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
Looks like the change to use nullptr is the cause of conflicts. Could this perhape be changed to instead extend the existing newgame command such that the second parameter can be a I.e.: And perhaps also allow |
Yet another conflict? I rebased 4 days ago to remove them already... sigh As for you suggestion of using flags/options, it is unheard of in console commands as far as I know: https://wiki.openttd.org/Console_Commands If there are global design decisions voted as part of a 'grand plan', I would be then gladly following them. Unless I am wrong, so far all I saw were individual decisions here and there, which IMHO hurt a community project. No command showing sign of options, I am concluding it has not been the followed design so far. |
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
5420a7f
to
d0e402e
Compare
|
||
/* Unfortunately, initializing file type to being invalid breaks the SafeLoad function which expects SLO_LOAD operation: initializing to dummy value */ | ||
/*_file_to_saveload.SetMode(FIOS_TYPE_INVALID); | ||
_file_to_saveload.SetMode(SLO_INVALID, FT_INVALID, DFT_INVALID);*/ |
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.
This commented out code should be removed
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.
This comment underlines a problem which might be past my understanding.
Is the dummy value OK?
Shall I use FIOS_TYPE_INVALID
instead? If so, is there a problem with SafeLoad
? If not, what am I doing wrong?
} | ||
} | ||
|
||
/* If supplied value was not recognized as a valid resource, attempt to find a matching title */ |
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 don't think loading by title is a good idea, especially not from the commandline (could result in confusing behaviour). Just filename will do, IMO
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.
Is there a consensus on that?
I find this feature quite handy & nice
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.
This PR grew a bit big :D Making it very difficult to review, as several different conversations go through each other now. This is a good example of that.
CLI tools should just use filenames; that is what users of CLI are used to, and going against that is only asking for problems. I am also not sure what issue it should fix to do it like that.
But as mentioned, if you want to have this in OpenTTD, please make it another PR, as having a conversation in a conversation makes for poor conversations :D
Tnx!
I spend the last 4 hours or so going up and down the rabbithole that is called our console code. I found that it is difficult, and I think this PR is not the right approach. Mostly, the issue is that Additionally, we know for a while that Next, I looked at the command itself, and found out that I would expect Lastly, this PR had a bit of a feature-creep, where it also changed how So in conclusion, and I do realise this is almost 2 years after the PR was created, sorry about that, I am going to close this PR. First someone needs to figure out what we want with the console, and what we want with file listing, and we can work up from there to support loading heightmaps and scenarios next to the savegames. tl;dr: this PR does too many things, and the |
I am... speechless. Being no developer, this PR (amongst the only few I made) was the most advanced one, feature-wise. It would not be an exaggeration to state that I was somewhat proud about it. From a practical point of view, it all started because I wanted to improved the dedicated server version of the game, making it able to load heightmap games (a feature which is, to this day, still lacking), although the heightmap format is very powerful, practical and existing in the game for a long time now. It felt wrong it was unusable in dedicated servers. I understand the feature creep feeling, and I would have been more than eager to work with you on that, especially on the request to split this work in multiple PR if you saw that fit. I understand the whole command-line thingy needs to be revamped, but IMHO it is no reason to reject attempts at iterating over it. Nothing can't be refactored later to follow yet-to-exist clear specifications. I understand the code is a bit messy and convoluted, but again I did my best on the cadebase as it was. Again, I am no developer: if understanding the codebase was difficult for you, imagine what it was to me... and after much effort I was able to reach some working state, step by step. I also understand the work I produced might not be of the highest quality standard, and maybe I myself introduced some off behaviour due to lack of skills and/or understanding of the codebase, but this is not an impossible issue to address. To each problem its adaquate tackle. Piling on multiple problems (most of them beyond my reach) as a reason to send that contribution attempt away in a snap feels like being unfair. I will continue running my modified servers, and I hope that, once the required groundwork to clean up the CLI interface is made, you will remember about this PR attempt and extract its main idea to implement it. As of now, with that PR closed, all I see is a long-lacking feature which has been killed. Happy 2021, and so long. |
Work inspired by the latest patch from old FlySpray bug tracker: https://bugs.openttd.org/task/2155/getfile/8568/Heightmap_Console_command.patch in #2155:
newheightmapgame
command accepting up to 2 parameters (file name, title or index to load - mandatory, and an optional seed)Sadly, there is no clean way to have a 2-arguments version where the second one would be the seed, as it would conflict with the 2-arguments version where the integer represent the file ID. Even if file ID was to be discarded, there would be some complicated and dangerous checks to check if the 2nd parameter is a string or an integer that could potentially introduce vulnerabilities.