Ancurio / mkxp

Free Software implementation of the Ruby Game Scripting System (RGSS)
GNU General Public License v2.0
509 stars 128 forks source link

Convert nil int/float arguments to 0 #227

Open Cloudef opened 4 years ago

Cloudef commented 4 years ago

Some games pass nil as bgs/bgm play pos arg :/

cremno commented 4 years ago

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.

Cloudef commented 4 years ago

@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.

Cloudef commented 4 years ago

Fixed formatting.

cremno commented 4 years ago

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.

Cloudef commented 4 years ago

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.

Cloudef commented 4 years ago

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.

cremno commented 4 years ago

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.

Ancurio commented 3 years ago

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.