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

Add #2155: newheightmapgame command #7286

Closed
wants to merge 3 commits into from
Closed

Conversation

Berbe
Copy link
Contributor

@Berbe Berbe commented Feb 26, 2019

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:

  • Added newheightmapgame command accepting up to 2 parameters (file name, title or index to load - mandatory, and an optional seed)
  • The short version with no argument reloads the current heightmap with a random 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.

@Berbe Berbe marked this pull request as ready for review February 26, 2019 21:27
Copy link
Contributor

@planetmaker planetmaker left a 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

src/console_cmds.cpp Outdated Show resolved Hide resolved
src/genworld_gui.cpp Outdated Show resolved Hide resolved
@Berbe Berbe changed the title Add newheightmapgame command (addresses #2155) Add newheightmapgame command (addresses #2155) Feb 26, 2019
@Berbe Berbe changed the title Add newheightmapgame command (addresses #2155) Add newheightmapgame command (addresses #2155) Feb 26, 2019
@Berbe Berbe changed the title Add newheightmapgame command (addresses #2155) Add #2155: newheightmapgame command Feb 26, 2019
@Berbe Berbe force-pushed the heightmap branch 3 times, most recently from e61d48b to 5733a73 Compare February 26, 2019 21:59
planetmaker
planetmaker previously approved these changes Feb 28, 2019
PeterN
PeterN previously requested changes Feb 28, 2019
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Outdated Show resolved Hide resolved
src/console_cmds.cpp Show resolved Hide resolved
@Eddi-z
Copy link
Contributor

Eddi-z commented Feb 28, 2019

the name sounds a bit clumsy, maybe newgame_heightmap would be a little clearer? not sure how that relates to other existing commands.

@Berbe
Copy link
Contributor Author

Berbe commented Feb 28, 2019

the name sounds a bit clumsy, maybe newgame_heightmap would be a little clearer? not sure how that relates to other existing commands.

There is a newgame command. Out of lack of creativity, I merely reused the newheightmapgame command name as suggested in the original patch.
How do we decide what's best? 😉

src/console_cmds.cpp Outdated Show resolved Hide resolved
@Berbe Berbe force-pushed the heightmap branch 4 times, most recently from dc7a1cd to 3364161 Compare March 3, 2019 13:52
@Berbe
Copy link
Contributor Author

Berbe commented Mar 3, 2019

Added a new feature commit to allow CLI loading of heightmaps by filename/name/title

src/openttd.cpp Outdated Show resolved Hide resolved
@glx22
Copy link
Contributor

glx22 commented Mar 4, 2019

Made some suggestions in Berbe#1

@stale
Copy link

stale bot commented Apr 5, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale Stale issues label Apr 5, 2019
@stale stale bot removed the stale Stale issues label Apr 10, 2019
@nielsmh
Copy link
Contributor

nielsmh commented Apr 12, 2019

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 -h flag, instead of adding a new command?

I.e.:
newgame scenarioname
newgame -h heightmapname

And perhaps also allow newgame -s scenarioname for consistency.

@Berbe
Copy link
Contributor Author

Berbe commented Apr 13, 2019

Yet another conflict? I rebased 4 days ago to remove them already... sigh
I will see to it.


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
'console' might be confusing, but we are not talking command-line, rather in-game console commands which follow far simpler syntax.

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.
To avoid adding to the entropy, diverging from what actually exist, I will so far stick with the current way console commands are, anyone being free sharing an implementation of a more complex system.

@stale
Copy link

stale bot commented May 13, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale Stale issues label May 13, 2019
@stale stale bot removed the stale Stale issues label May 14, 2019
@LordAro LordAro requested a review from PeterN August 17, 2019 20:46
@Berbe Berbe force-pushed the heightmap branch 3 times, most recently from 5420a7f to d0e402e Compare February 10, 2020 20:16

/* 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);*/
Copy link
Member

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

Copy link
Contributor Author

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

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

Copy link
Contributor Author

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

Copy link
Member

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!

@TrueBrain TrueBrain added candidate: most likely This Pull Request is probably going to be accepted size: large This Pull Request is large in size; review will take a while labels Dec 14, 2020
@TrueBrain TrueBrain added the work: needs more work This Pull Request needs more work before it can be accepted label Dec 24, 2020
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
@TrueBrain
Copy link
Member

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 ls only lists savegames. So I am not really sure how this was suppose to work, but when I do newheightmapgame 1, I have no idea which heightmap it is about to load :D So after some staring at the code, and trying various of things, I came to the conclusion there isn't really a way that is clean and clear to do this.

Additionally, we know for a while that ls and cd in the console are just a bad idea. So I think we should just change how you can list savegames etc, like: list savegames and list scenarios or something, and drop the navigating of files. Basically saying: if a savegame is not in the search path .. well .. maybe we shouldn't allow loading it from the console. But that is a bit outside this PR, but required for the PR in order the work.

Next, I looked at the command itself, and found out that I would expect load could also load scenarios, and possibly heightmaps. The latter is always a bit difficult, as you start a new game based on a heightmap .. do you really load it, is the question. But okay, semantics left there, this is something we can perfectly explain in the help, If people really object to that, I agree with a comment made in this PR: use newgame -h <id> or something. That would be much more intuitive.

Lastly, this PR had a bit of a feature-creep, where it also changed how -g works. I am very much against that change, for the simple reason that if you want -g to also work with "friendly" names, you should also allow a way to list them via the CLI. And all that code, I have to ask, what does it add? How much of a problem is using the filename over a "friendly" name in that case. It would only make sense if it was tight to the content service and could also download it, or something. Either way, this is just too much for this PR.

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 newheightmapgame command is in a practical sense not usable as presented in this PR. We need to go back a bit and rework how the console works for this to make sense.

@TrueBrain TrueBrain closed this Jan 8, 2021
@Berbe
Copy link
Contributor Author

Berbe commented Jan 9, 2021

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.
Also from a practical point of view, I am using the code I pushed to you on my own dedicated server instances, which have been running flawlessly for a long time.

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.
It is a bit overwhelming as, after 2 years, I am myself a bit rusty on all the details I put in those 3 orderly (or so I thought) commits.
I certainly did not expect it to end up being washed down the drain like that. In those 2 years, even with sporadic back-and-forth reviews and corrections, we could have achieved much. Instead, this eulogy followed by the trashbin. I am sorry we collectively spent that much time, involving that much people on this contribution attempts: this time is effectively lost for everyone.

Happy 2021, and so long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: most likely This Pull Request is probably going to be accepted size: large This Pull Request is large in size; review will take a while work: needs more work This Pull Request needs more work before it can be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants