verify command line options #650

rfomin · 2022-07-10T07:38:49Z

Inspired by this issue: coelckers/prboom-plus#515

I'm using docgen to generate a list of command line options to check.

fabiangreffrath · 2022-07-10T07:49:29Z

src/m_argv.c

+    if (CheckParamsWithArgs(i))
+    {
+      // -file and -warp may have multiple arguments
+      if (!strcasecmp(myargv[i], "-file") ||


Does -deh also take multiple args?

-warp can take one or two arguments, depending on the version. Not sure if we should check everything.

Hm, "do it right or don't do it". 😁 Honestly, I'd expect command line checking code to detect that -warp 1 2 3 4 5 cannot be right.

Hm, "do it right or don't do it".

Yes, that's the question 😄 How do you like this approach, is it worth finishing it?
There is also -recordfrom which only takes two arguments.

BTW with docgen we can also generate code for --help options list, or maybe even help <option> messages.

We will also need to check the validity of the parameter passed to e.g. the -complevel argument, else this whole checking procedure won't make too much sense.

I think we should check the validity of -complevel and other arguments in the game code, such as the -file argument if the path/file is not found. There is 36 options with arguments and most of them silently ignore wrong input.

kraflab · 2022-07-10T14:05:30Z

kraflab/dsda-doom@74a25a5 maybe some food for thought, an approach in dsda-doom (I see this as a more important precursor, but checking if they are used is easy to add on top).

rfomin · 2022-07-10T15:14:14Z

kraflab/dsda-doom@74a25a5 maybe some food for thought, an approach in dsda-doom (I see this as a more important precursor, but checking if they are used is easy to add on top).

Interesting, thanks. Looks like PrBoom+'s complevel is the only option that allows a negative integer as an argument? I want to use the - character as a delimiter.

kraflab · 2022-07-10T17:32:03Z

Looks like PrBoom+'s complevel is the only option that allows a negative integer as an argument?

-spechit would "work" with a negative number I think, although I don't really know what the implication would be. It's an address offset to use for certain overflow problems in some specific demos :^)

-setmem also works with negative numbers. It's another overrun thing that I haven't looked into :^)

-skipsec takes negative numbers to skip from the end of the demo instead of from the start

There may be other stuff that takes -1 for a default, I have no idea yet (but I guess I will have a full list when my refactoring is done...)

rfomin · 2022-07-10T17:52:06Z

-skipsec takes negative numbers to skip from the end of the demo instead of from the start

Ha, I didn't know that and implemented it differently.

As for memory overflows, I think they are only positive, at least in the Choco code we copied, it is unsigned.

kraflab · 2022-07-10T22:45:40Z

src/d_main.c

+      }
+     else
+      {
+        I_Error("Wrong -episode parameter '%s', should be 1-4", myargv[p+1]);


Can't we have any episode from 0 to 9 now?

Right, in Woof we even support episodes 0-99.
@fabiangreffrath BTW -warp 9999 doesn't currently work, only -warp 99 99. Not sure how to validate -warp 😄

I think it's okay as it is. No algorithm can know if -warp 999 means to warp to E99M9 or E9M99. 😉

fabiangreffrath · 2022-07-12T09:50:30Z

From my testing -skill 0 doesn't work at all, all monsters and items still spawn.

This should emulate Vanilla behaviour:

--- a/src/p_mobj.c
+++ b/src/p_mobj.c
@@ -1209,10 +1209,11 @@ void P_SpawnMapThing (mapthing_t* mthing)
     return;

   // killough 11/98: simplify
-  if (gameskill == sk_baby || gameskill == sk_easy ?
+  if ((gameskill == sk_none && demo_version < 203) ||
+      (gameskill == sk_baby || gameskill == sk_easy ?
       !(mthing->options & MTF_EASY) :
       gameskill == sk_hard || gameskill == sk_nightmare ?
-      !(mthing->options & MTF_HARD) : !(mthing->options & MTF_NORMAL))
+      !(mthing->options & MTF_HARD) : !(mthing->options & MTF_NORMAL)))
     return;

   // [crispy] support MUSINFO lump (dynamic music changing)

fabiangreffrath · 2022-07-12T09:55:40Z

This should emulate Vanilla behaviour:

Though, even Pr+ doesn't emulate this. But then the parameter help string needs to be adjusted.

rfomin · 2022-07-12T10:13:54Z

Though, even Pr+ doesn't emulate this. But then the parameter help string needs to be adjusted.

We need to emulate it as we support Choco netgame so there is a possibility of desync. Interesting that skill 0 doesn't work in demos.

kraflab · 2022-07-12T22:58:20Z

kraflab/dsda-doom#116 if you're curious how that went🥵
+1526 -936, slightly intrusive change :^)
Refactoring the use of 118 arguments is a real test of willpower 🤠
But, never seeing p < myargc - 1 in the code base is 💆

rfomin · 2022-07-13T02:12:31Z

+1526 -936, slightly intrusive change :^)
Refactoring the use of 118 arguments is a real test of willpower 🤠
But, never seeing p < myargc - 1 in the code base is 💆

I think Woof's motto is to preserve as much code as possible, so I take the simpler approach 😄 We have 82 options, but there are a few undocumented secret ones.
Not sure if people will be happy with this change, it might break some launchers/.bat files.

rfomin · 2022-07-13T02:21:36Z

man/docgen

+
+        result += " " * (indent - len(result))
+
+        result += textwrap.fill(self.text, width = (90 - indent),


@fabiangreffrath Should we wrap help text for console output? Maybe we should write shorter help lines instead 😉

Help text should fit into one console line ideally. Do you have an example of a help text line that doesn't fit?

Oops missed your answer.
This is about a response to the -help command. I just generate our CMDLINE documentation, but with selected categories and parameters. So we have a lot of long lines here with multiple sentences: help.txt

Oh, I like this very much! However, 90 chars is too long for a terminal, e.g. MSYS2's mintty defaults to 80x24. To achieve this, I think we shouldn't cut down the help strings. I very much prefer them to be real sentences and actually helpful. Could you tune it so that each line is broken after at most 80 chars?

Yes, 80 columns is the standard, but I have noticed that many modern applications do not respect it.
Anyway, my English is bad, so I'll just let the Python textwrap library do its job 😄

kraflab · 2022-07-13T20:50:04Z

src/i_system.c

+                    {
+                        I_Error("Invalid parameter '%s' for -setmem, "
+                                "valid values are dos622, dos71, dosbox "
+                                "or memory offset.", myargv[p]);


It's not an offset is it? I think it's up to 10 bytes of data.

You're right, I'll correct it.

fabiangreffrath · 2022-07-14T06:21:16Z

man/docgen

+
+    for t in targets:
+        # no video and obscure category
+        if t != c["video"] and t != c["obscure"]:


Why no video?

I don't think we have a video option important enough to include in the -help output. However, I'm not sure.

Okay, the most important options already have their switches in the menu.

src/d_main.c

fabiangreffrath · 2022-07-14T06:24:33Z

src/d_main.c

-      startmap = 1;
-      autostart = true;
+      startepisode = M_ParmArgToInt(p);
+      if (startepisode >= 0 && startepisode <= 99)


We are still unconditionally restricting episode to be > 0:

woof/src/g_game.c

Lines 2982 to 2983 in d8a068c

if (episode < 1)

episode = 1;

fabiangreffrath · 2022-07-14T06:55:25Z

So, is this ready for merging now?

rfomin · 2022-07-14T07:03:30Z

Yes, it's ready.

rfomin added 2 commits July 10, 2022 14:19

verify command line options

2a39532

fix cmake

11498c9

fabiangreffrath reviewed Jul 10, 2022

View reviewed changes

rfomin added 2 commits July 10, 2022 14:54

add -deh -bex to multiple options check

beefac0

fix typo

2e24294

rfomin added 2 commits July 11, 2022 00:36

fix parameter count check

e2c901e

add validation for some arguments

67f1f14

fix skill 0

be53c42

kraflab reviewed Jul 10, 2022

View reviewed changes

validate all parameters with argumets

25381d5

rfomin marked this pull request as ready for review July 12, 2022 05:58

rfomin added 6 commits July 12, 2022 13:28

cosmetic fixes

077782b

don't check undocumented options if -devparm is set

ed89214

fix format error

1b13549

fix whitespace

38e0fb7

docgen cleanup

37896b8

rename GenParams->paramsgen

d637a8a

rfomin added 5 commits July 12, 2022 20:35

fix -turbo check

6348fea

more docgen cleanup

53030ca

generate help string

f3705c3

use dictionary

86affe6

condense help text

9152342

rfomin commented Jul 13, 2022

View reviewed changes

rfomin added 4 commits July 13, 2022 09:45

escape "

c163642

remove -gameversion from help string

666b1af

set wrap width to 80, few tweaks

59273d9

remove debug line

1b8846f

kraflab reviewed Jul 13, 2022

View reviewed changes

-setmem error message correction

96a1cbe

fabiangreffrath reviewed Jul 14, 2022

View reviewed changes

src/d_main.c Show resolved Hide resolved

fabiangreffrath reviewed Jul 14, 2022

View reviewed changes

rfomin added 2 commits July 14, 2022 13:44

fix -episode check and -skill description

cdd7dc1

little cleanup

34c5e2f

fabiangreffrath merged commit 399e88c into fabiangreffrath:master Jul 14, 2022

rfomin deleted the check_cmdline branch July 14, 2022 07:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verify command line options #650

verify command line options #650

rfomin commented Jul 10, 2022

fabiangreffrath Jul 10, 2022

rfomin Jul 10, 2022

fabiangreffrath Jul 10, 2022

rfomin Jul 10, 2022

rfomin Jul 10, 2022

fabiangreffrath Jul 10, 2022

rfomin Jul 10, 2022

kraflab commented Jul 10, 2022 •

edited

Loading

rfomin commented Jul 10, 2022

kraflab commented Jul 10, 2022

rfomin commented Jul 10, 2022

kraflab Jul 10, 2022

rfomin Jul 11, 2022

fabiangreffrath Jul 11, 2022

fabiangreffrath commented Jul 12, 2022 •

edited

Loading

fabiangreffrath commented Jul 12, 2022

rfomin commented Jul 12, 2022

kraflab commented Jul 12, 2022

rfomin commented Jul 13, 2022 •

edited

Loading

rfomin Jul 13, 2022

fabiangreffrath Jul 13, 2022

rfomin Jul 13, 2022 •

edited

Loading

fabiangreffrath Jul 13, 2022

rfomin Jul 13, 2022

kraflab Jul 13, 2022

rfomin Jul 14, 2022

fabiangreffrath Jul 14, 2022

rfomin Jul 14, 2022

fabiangreffrath Jul 14, 2022

fabiangreffrath Jul 14, 2022

fabiangreffrath commented Jul 14, 2022

rfomin commented Jul 14, 2022


		result += " " * (indent - len(result))

		result += textwrap.fill(self.text, width = (90 - indent),

verify command line options #650

verify command line options #650

Conversation

rfomin commented Jul 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraflab commented Jul 10, 2022 • edited Loading

rfomin commented Jul 10, 2022

kraflab commented Jul 10, 2022

rfomin commented Jul 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiangreffrath commented Jul 12, 2022 • edited Loading

fabiangreffrath commented Jul 12, 2022

rfomin commented Jul 12, 2022

kraflab commented Jul 12, 2022

rfomin commented Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfomin Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiangreffrath commented Jul 14, 2022

rfomin commented Jul 14, 2022

kraflab commented Jul 10, 2022 •

edited

Loading

fabiangreffrath commented Jul 12, 2022 •

edited

Loading

rfomin commented Jul 13, 2022 •

edited

Loading

rfomin Jul 13, 2022 •

edited

Loading