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

Open
wants to merge 3 commits into
base: master
from
Open

Add #2155: newheightmapgame command #7286

wants to merge 3 commits into from

Conversation

@Berbe
Copy link
Contributor

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 force-pushed the Berbe:heightmap branch from 9924659 to d74fb43 Feb 26, 2019
@Berbe Berbe marked this pull request as ready for review Feb 26, 2019
Copy link
Contributor

planetmaker left a comment

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 Berbe:heightmap branch 3 times, most recently from e61d48b to 5733a73 Feb 26, 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

This comment has been minimized.

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

This comment has been minimized.

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? 😉

@Berbe Berbe force-pushed the Berbe:heightmap branch 6 times, most recently from 16cf7bf to 81b504d Mar 1, 2019
src/console_cmds.cpp Outdated Show resolved Hide resolved
@Berbe Berbe force-pushed the Berbe:heightmap branch 4 times, most recently from dc7a1cd to 3364161 Mar 2, 2019
@Berbe

This comment has been minimized.

Copy link
Contributor Author

Berbe commented Mar 3, 2019

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

@Berbe Berbe force-pushed the Berbe:heightmap branch from 3364161 to c8b0a91 Mar 3, 2019
src/openttd.cpp Outdated Show resolved Hide resolved
@Berbe Berbe force-pushed the Berbe:heightmap branch from c8b0a91 to ffe7abf Mar 3, 2019
@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Mar 4, 2019

Made some suggestions in Berbe#1

Berbe added a commit to Berbe/OpenTTD that referenced this pull request Mar 4, 2019
Thanks for that @glx22!
* Codechange: simplified _switch_mode handling

The name of that variable is bothering me, though... I am over-conservative sometimes.
I will work on top of that.

* Fix: coding style

You are right, and I should normally have done this... I have however become wary of indentation following remarks of other contributors 😕
@Berbe Berbe force-pushed the Berbe:heightmap branch from 1216d04 to 77093af Mar 4, 2019
Copy link
Contributor

glx22 left a comment

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

@Berbe

This comment has been minimized.

Copy link
Contributor Author

Berbe commented Mar 4, 2019

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

That would kill the resemblance to StartNewGameWithoutGUI() which has a procedure signature.
If a check needs to be done, I guess we should rather modify/replicate SafeLoad to handle heightmaps.

@glx22

This comment has been minimized.

Copy link
Contributor

glx22 commented Mar 4, 2019

Something like glx22@7c3adf3 should do it (modulo error message and comments)

@Berbe Berbe force-pushed the Berbe:heightmap branch from 77093af to a00ceb7 Mar 4, 2019
@Berbe

This comment has been minimized.

Copy link
Contributor Author

Berbe commented Mar 4, 2019

The problems I see with your solutions are multiple:
1.StartNewHeightMapGameWithoutGUI signature has to be changed from void to bool, drifiting away from StartNewGameWithoutGUI
2. On success, using that function to check will actually also generate a new map, which makes the game generate the map twice in total
To prevent that, further modifying the signature to add some bool check parameter would be required, drifting further

I chose another way: including specialized heightmap.h functions in dedicated_v.cpp and calling directly the already existing GetHeightmapDimensions function which checks for file validity (as it is already used for at https://github.com/Berbe/OpenTTD/blob/heightmap/src/genworld_gui.cpp#L836).
I do not know the side-effects of that inclusion, nor if it is wished, but it looks overall cleaner.

I'll let reviewers be the judge of that.

@Berbe Berbe force-pushed the Berbe:heightmap branch 4 times, most recently from 775bac4 to d5d9ca4 Mar 5, 2019
@stale

This comment has been minimized.

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 label Apr 5, 2019
@Berbe Berbe force-pushed the Berbe:heightmap branch from d5d9ca4 to 291c527 Apr 10, 2019
@stale stale bot removed the stale label Apr 10, 2019
@nielsmh

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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 label May 13, 2019
@Berbe Berbe force-pushed the Berbe:heightmap branch from 291c527 to 340a1c2 May 14, 2019
@stale stale bot removed the stale label May 14, 2019
@LordAro LordAro requested a review from PeterN Aug 17, 2019
@LordAro LordAro dismissed PeterN’s stale review Nov 23, 2019

outdated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.