PrBoom doesn't throw error when incorrect command line parameters are supplied.

[kvsari]

PrBoom doesn't exit with an error when an incorrect command line parameter is supplied.

This breaks expected command line UI norms. When an incorrect parameter is supplied the program should exit with an error. Instead PrBoom merrily carries on. This is most problematic with the -record option.

There have been many times when I wanted to record a playthrough I may mistype the -record option or accidentally drop th -. PrBoom (running in fullscreen mode) plays as normal and after the play session is done I think a demo has been recorded until I find out it hasn't due to the mistyped command line parameter.

PrBoom should exit with an error in this case.

I have also detected this problem with mistyped -skill command line parameter too. PrBoom continues execution when it should exit with an error.

[JadingTsunami]

This one of those things that sounds trivial but when you look at the code...oof. The command line argument processing appears to have evolved over many years. There are tons of unrelated entry points for command line arguments that appear to have been added in an ad-hoc, as-needed manner. There are also "clean-up" routines that attempt to make sense of arguments that are supplied out-of-order by adding some implied context. I believe this is primarily to handle "drag and drop" use cases.

I've got one potential start of a solution in a branch here, but it has downsides. Most notably, for an unused argument to be detected, it must start with '-'. Otherwise, we'll need to either untangle the various ad-hoc command line parsers or add a bunch of "set a flag" calls inside them.

Of course we can also have options to ignore unused commands so users can restore the existing functionality if desired (possibly we could have this on by default). I'm sure some shell script or batch file out there will start erroring out if we deployed this as-is.

[kraflab]

I've been planning on refactoring command line parameter management in dsda-doom at some point, but this feels like too big of a change to add to pr+. One issue I noticed in that branch is that it doesn't work with parameter overrides - and there's no consistent way to know if they make sense. The current behaviour may not be ideal for all users but it's the way it has been forever, so I don't feel like it's a mistake that necessarily needs fixing right now - given that we are in maintenance mode.

[JadingTsunami]

One issue I noticed in that branch is that it doesn't work with parameter overrides - and there's no consistent way to know if they make sense.

Right, I'm basically side-stepping the issue by looking for '-' at the start of a parameter. Unless you mean something else, in which case, please do elaborate, I'd like to know more.

[kraflab]

Right, I'm basically side-stepping the issue by looking for '-' at the start of a parameter. Unless you mean something else, in which case, please do elaborate, I'd like to know more.

What I mean is that -warp 1 -warp 2 is currently valid. And -file something.wad -file something_else.wad. But was it an accident that they overrode the command or was it intentional? 😄

I have a batch file with default args for instance, and the file takes extra args and appends them. So it may have -skill 4 but I call the file with my_batch -skill 5 to play on nightmare, yielding -skill 4 -skill 5 which in practice becomes -skill 5, but it's intended.

[kraflab]

Another thing I just thought about, does the branch work with -complevel -1? :^)
In order to know if -1 is a new option or a value for the previous one, you need to know that -complevel is a parameter that requires one integer value, which means adding comprehensive validation information potentially for all options. This is the approach I will take in dsda-doom at least.

[JadingTsunami]

Ah, I see. Maybe some framing is in order. From what I understand, the request here comes from frustrations when command like parameters, usually "-record" for a demo, are accidentally mistyped. The player thinks they're recording a demo and then their many minutes or hours of gameplay footage is lost. I'm sure this feels like disproportionate punishment for a single finger-stumble while starting up.

This problem doesn't have to be solved by command-line verification. A visual indicator that demo recording has started would work as well. But let's put that aside for a moment.

I see three distinct problems here:

1. Did each component of the command line match to an appropriate parameter handler?
2. Was each parameter handler given valid inputs?
3. Taken together, did all the parsed commands make sense? In other words, does the complete command line make sense when viewed as a whole?

Sounds like in dsda-doom, all three problems are intended to be solved. That's a larger change than what I think is desired here.

The branch I've proposed intends to address the first item above only. It does this because any "loose" command line arguments are passed to a general purpose handler which attempts to load them as files. There are other parts of the code that use this convention as well: parameters must start with "-" else they are considered "loose files."

All in all, this seems like a simple and small enough change that would provide enough end benefit to players that it seems reasonable to me to include. We can always fence it with a config file option if desired.

EDIT: The "complevel -1" part is a good catch. The broader point about needing full input validation to catch every edge case is correct, and if we can maintain the current behavior for otherwise-correct command lines I think we can put those cases aside for what we want to solve here.

[kraflab]

My default stance is "no change that isn't a bug fix".
PRBoom+ is supposed to be pretty much unchanging - if this behaviour is changed, it will definitely throw errors for people that had "wrong" options somewhere that didn't happen to matter, which would technically be a breaking change.

To be honest, I think it might be preferable to just add player->message = "Recording started" somewhere, which would be low impact and fix the main point of stress here 😸

[JadingTsunami]

To be honest, I think it might be preferable to just add player->message = "Recording started" somewhere, which would be low impact and fix the main point of stress here 😸

I have to say, I kind of like this idea as a solution. While it does side-step the thorny issues with command line processing I think it gives players what they need and doesn't break anything.

[fabiangreffrath]

Very much yes! 👍

[JadingTsunami]

Something like this?

[fabiangreffrath]

Something like this?

I want this! Though I'm not sure if we need the string to be dehackable/translatable

[JadingTsunami]

I want this! Though I'm not sure if we need the string to be dehackable/translatable

Great! I didn't either. But I thought perhaps it was nicer to give the option rather than force a hardcoded string into the user's view. I'll file a PR where we can discuss more.