WeiDUorg / weidu

WeiDU is a program used to develop, distribute and install modifications for games based on the Infinity Engine.
http://www.weidu.org
GNU General Public License v2.0
89 stars 20 forks source link

Function defaults override vars inherited from environment #160

Closed burner1024 closed 4 years ago

burner1024 commented 4 years ago

Quoting doc:

Notice that arguments are passed indirectly to the function. It inherits the variable environment from the caller. So when you launch it, all variables that are visible at that point are also visible from within the function. You can provide default values for variables with INT_VAR (integers) and STR_VAR (strings). If the variable was not defined in the caller’s environment the default value is used

Do I just fail to read the manual or something is horribly wrong?

BACKUP ~1/backup~
AUTHOR "burner1024 @ github"
VERSION ~1~
README ~%MOD_FOLDER%/readme.txt~
AUTO_EVAL_STRINGS

BEGIN ~test~
DEFINE_PATCH_FUNCTION count_spells INT_VAR level = 1 STR_VAR type = "WI" RET num BEGIN
  SET num = 0
  PATCH_PRINT "level is %level%, type is %type%, test is %test%"
END
COPY_EXISTING ~jaheir12.cre~ ~override~
  // count number of level 1 wizard spells (default arguments of the function)
  LAUNCH_PATCH_FUNCTION count_spells RET lvl1_wizard = num END
  // and now the level 5 priest spells
  SET level = 5
  SPRINT type ~PR~
  test = 999
  LAUNCH_PATCH_FUNCTION count_spells RET lvl5_priest = num END
  PATCH_PRINT ~%SOURCE_FILE% has %lvl1_wizard% level 1 wizard spells and %lvl5_priest% level 5 priest spells~
BUT_ONLY
[weidu] WeiDU version 24600

Couldn't open the readme: file not found.

Install Component [test]?
[I]nstall, or [N]ot Install or [Q]uit? i

Installing [test] [1]
Copying and patching 1 file ...

level is 1, type is WI, test is %test%

level is 1, type is WI, test is 999

jaheir12.cre has 0 level 1 wizard spells and 0 level 5 priest spells

SUCCESSFULLY INSTALLED      test

Press ENTER to exit.
4Luke4 commented 4 years ago

@burner1024

Try this:

SET level = 5
SPRINT type ~PR~
test = 999

LAUNCH_PATCH_FUNCTION count_spells
INT_VAR
  level
STR_VAR
  type
RET
  lvl5_priest = num
END

Quoting doc:

// Starting from V210

OUTER_SET y = 5
LAUNCH_ACTION_FUNCTION xyz INT_VAR y END // synonym for INT_VAR y = y
burner1024 commented 4 years ago
level is 1, type is WI, test is %test%

level is 5, type is PR, test is 999

That works, but if I understand the doc correctly, the first way must work as well.

4Luke4 commented 4 years ago

That works, but if I understand the doc correctly, the first way must work as well.

Well, it seems it's necessary to take that extra step (i.e., you need to specify the variable with no value – as it's written at the end of section 10.23)

The first way works just fine in Matlab, for instance...

FredrikLindgren commented 4 years ago

@burner1024 I'm afraid I do not understand. Your code does not set test before you set it to 999, so when you call the function the first time, you would get unset-variable behaviour when you try evaluating test. Am I missing something?

Inheriting variables works as described, as far as I can tell. You use it the second time you call the function and test evaluates to 999.

Edit: oh, maybe you mean type evaluating to "WI" the second time.

If the variable was not defined in the caller’s environment the default value is used

You use a default value of "WI" for type, which, as per the doc (though I would agree it is ambiguously written, perhaps outright incorrect [default values did not override originally, and this may be a hold-over from those times; I'll clear up the language]), overrides an inherited variable value. What the above quoted sentence says (Edit2: or should say) is that unless your function call has a STR_VAR type = foo of some kind, the default value is used.

Edit 3: yeah, I apologise for not getting it right away. That tutorial is outright incorrect (and was probably indeed written in the dark days); will fix.

burner1024 commented 4 years ago

Edit 3: yeah, I apologise for not getting it right away. That tutorial is outright incorrect (and was probably indeed written in the dark days); will fix.

But I like the dark days... What I mean to say is that I don't know what was the reason for this decision in the first place, and I don't really see the difference, functionality wise. But having to explicitly specify all arguments for a function does get aggravating. (See an example of ugliness it produces). If people have to specifically work around a feature, maybe the feature isn't that great).

Seeing that this is not a bug per se, maybe it would it be possible to add a flag such as FUNCTIONS_REALLY_INHERIT_ENVIRONMENT to let outer environment override function defaults instead?

FredrikLindgren commented 4 years ago

You can't really have sane and reusable functions if variable defaults are overridden by whatever's in the calling environment. If you want to capture variables from the local environment, I consider the current syntax reasonable for the purpose. It's comparably succinct and it makes the capture explicit rather than implicit. Edit: and "the dark days" were something like a decade ago.

INT_VAR
  foo // short for foo = foo
STR_VAR
  bar // short for bar = EVAL "%bar%"

You may consider omitting the default value if you want this behaviour.

burner1024 commented 4 years ago

I think the example I showed proves it not being succint "enough". But I have nothing to say about sanity, so I defer to your judgement.