mkxp
mkxp copied to clipboard
Ancurio
Convert nil int/float arguments to 0
I have to admit I'm not a huge fan of mkxp's rb_get_args (the current implementation anyway). The RGSS is full of bugs and peculiarities including value conversion to C++ from Ruby and vice versa.
As you can see it doesn't care about T_BIGNUM at all. While their usage in RGSS based games is uncommon it usually can handle them. mkxp's rb_{int,float}_arg assigns a long to a int here too (should be NUM2INT!).
The RGSS peculiarity you've found and I didn't know about is that the BGM/BGS position argument can both be nil and a number. But here's the thing only position may be nil. Volume and pitch have to be a number. Your changes would allow them both to be also nil.
A preferable fix would be adding a mruby-like ! to i and maybe f (which mruby doesn't support) and change iii to iii!, or, since/if the special behavior is only seen in these two similar functions, removing the rb_get_args call and parse the arguments manually instead.
@cremno that makes sense. I'm not that familiar with ruby to begin with. But modelling the real constraints would avoid the problem of people targeting mkxp's implementation and then not running on real RPG maker runtime.
To re-phrase my comment above: While your PR improves compatibility with the original implementation, it also introduces incompatibilities at the same time. Something like Color.new(nil, nil, nil) wouldn't raise a TypeError anymore. Any potential fix should only affect the 4th argument to Audio.{bgm,bgs}_play.
Btw. I've looked at the relevant RPG classes (see the help file) and apparently position is only allowed to be nil because @pos may be uninitialized. In Ruby uninitialized instance variables default to nil. So it seems they've added a workaround to Audio.bgm_play hiding a bug instead of fixing it.
Because this isn't proper fix. This PR won't be merged I guess? I'm aware of this converting all nils to 0 implicitly. I could alternatively add support for ! syntax in rb_get_args, and fix this same method that way.
Another option would be to give rb_get_args a "type error" handler std::function where you could try to do conversion yourself, or reject it.
My comments are simply code review. Only @Ancurio got all the relevant powers. Maybe he doesn't like ! or converting the arguments 'manually' and prefers your callback function idea instead. But any of these approaches would squash the bug you've found without letting a new one creep in.
Sorry, I'll be getting around to this in a couple weeks. Thanks cremno for the pointers on rb_get_args, hopefully that can be addressed in a separate change set.
