Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert nil int/float arguments to 0 #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Cloudef
Copy link
Contributor

@Cloudef Cloudef commented Mar 25, 2020

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

@cremno
Copy link
Contributor

cremno commented Mar 25, 2020

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
Copy link
Contributor Author

Cloudef commented Mar 25, 2020

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

Some games pass nil as bgs/bgm play pos arg :/
@Cloudef
Copy link
Contributor Author

Cloudef commented Mar 25, 2020

Fixed formatting.

@cremno
Copy link
Contributor

cremno commented Mar 25, 2020

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
Copy link
Contributor Author

Cloudef commented Mar 25, 2020

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
Copy link
Contributor Author

Cloudef commented Mar 25, 2020

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
Copy link
Contributor

cremno commented Mar 25, 2020

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
Copy link
Owner

Ancurio commented Aug 11, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants