Some plotscripting commands return seemingly valid values when they fail

[ohrrpgce-bugbot]

[bz#36]

The command NPC Y(#) will return the ID number of the NPC referenced if no
copies of that NPC exist on the given map. It should return a value of -1 to
indicate that there are none of the referenced NPCs.

From: Mike Willis <msw188>
Reported version: 20040628 Ozarks
Operating system: Windows ME

[ohrrpgce-bugbot]

Comment author: @rversteegen

This isn't the only coomand that is affected. The majority of functions
thatreturn values are. Would it be a good idea to have a default
scriptret = -1
before executing a command? -1 may not be the correct error value for all commands

[ohrrpgce-bugbot]

Comment author: msw188

I'm pretty sure that most of the plotscripting commands would do alright with
a -1 return to indicate an error. Except the mathematics operations. What do
they do when there is a problem?

Hmm, actually maybe NPC reference would be trouble here. Aren't certain NPCs
referenced with negative numbers? Would a
scriptret = -1
mess this up at all? I still believe that having some default scriptret would
be a good idea.

[ohrrpgce-bugbot]

Comment author: @rversteegen

We planned to return 0 by default (actually, I'll just go throw that in - in a
minute). If this is not a good indicator of error, then the individual command
should return a special value.

The problem really is that currently it's nearly impossible to tell when you're
passing things in wrong, and if you are, a large number of plotscripting
commands bound your inputs to valid values. If you try to write to hero number
-1, you'll probably modify hero 0 instead. Nobody's going to check the return
value of every command they call. Better to always pass in valid arguments.

[ohrrpgce-bugbot]

Comment author: @bob-the-hamster

Just an idea on this subject. What about taking an idea from moldy old DOS batch files? There they have an ERRORLEVEL that gets set after each command. Suppose we had a command like "was error" which woyld return true if the previous command failed, and false if the previous command worked?

Not as slick as a try/except/finally block, but a heck of a lot easier to implement :)

[ohrrpgce-bugbot]

Comment author: @rversteegen

You know... if you want try/catch/finally, that could be done ;)

Except I've never used exceptions in my life.

[ohrrpgce-bugbot]

Comment author: @sorlok

Just an idea on this subject. What about taking an idea from moldy old DOS
batch files? There they have an ERRORLEVEL...

I was browsing bugzilla (like you do...) and I came across this bug. If you're considering DOS error levels, you might want to consider Win32's approach too:

1. Commands generally return some value for failure (like -1).
2. If a command failed, then calling GetLastError() will return some relevant error string/id.
3. Note that, if it's impossible to "return -1" as an error, calling GetLastError() will STILL return some non-zero value, indicating that an error occurred.

I like this approach, because it allows simple error checking via comparison (e.g., some_func() != -1), yet it also permits fine-grained detail when required. Having some wasError() function will clutter scripts a lot more than this.

By the way, regarding try/catch/finally; although I use Exceptions constantly in my programming, I tend to agree that they're not really the right way to do things in HSpeak. It just doesn't feel natural, in my opinion.

All the best,
-->Seth

[ohrrpgce-bugbot]

Comment author: @rversteegen

Having given this a few years of thought, I don't like either exception handling or errorlevels/GetLastError/WasError.

APIs usually suffer from horrible external uncertainties. Did another process just delete this file? Is the system shutting down? Did your dog just chew through the ethernet cable?

Compare to plotscripting: there are basically no uncertainties that you can't explicitly check for. You get test whether an NPC exists before calling NPC Y. You can check that a hero is in the intended party slot (really need slot-specific locking though). Or that a slice handle is still valid. One exception might be the joystick commands: we don't have a way to check whether the player unplugged their joystick in the middle of the game or anything like that (not that you would want to bother going to that trouble).

Having command-specific failure codes (either 0 or -1 in all cases I can think of) is OK - that's where we're already. Any command that can fail should (implicitly in the case of valid arguments) document all the ways that it can fail, and how to test for them. If a command can return anything as valid data, and can fail, then we need to have to way to check for failure (normally before running the command, so that we can raise a script error).

When types are added, we could return special error objects from commands. Some could be distinguishable from all integers (wouldn't do this for any existing commands for compatibility), others could automatically cast to an integer like 0 or -1 (basically the "weak types" I've currently described on the wiki, which I will replace with "objects with an automatic cast-to-int"). They would be recognisable with a "failure" function/predicate (and we could put extra info in them if do we decide that's needed).

[rversteegen]

I'm closing this, because there's no specific problem mentioned. The original bug was that scriptret wasn't zeroed out, causing garbage return values, and that's fixed.

A lot of commands (fewer than 15 years ago) have poor error checking, but they're not failing, they're doing something, and that's a different problem.

Having given this another decade of thought, I still like the idea of returning "weak types" from a lot of commands: objects that convert implicitly into integers, but can also store extra information like success/failure. Eg
success(find hero(hero:bob)))
equivalent to
find hero(hero: bob) <> -1
and
success(npc reference(3, 2))
equivalent to
npc reference(3, 2) <> 0