MapleShrine
/
mkxp
1
0
Fork 0
Code Issues 32 Pull Requests 11 Projects Releases Wiki Activity

Convert nil int/float arguments to 0 #227

Open
Cloudef wants to merge 1 commits from Cloudef/ruby-game-compatibility into master
pull from: Cloudef/ruby-game-compatibility
merge into: MapleShrine:master
Conversation 8 Commits 1 Files Changed 1 +10 -2
Cloudef commented 2020-03-25 06:29:11 +00:00 (Migrated from github.com)
Copy Link

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

Some games pass nil as bgs/bgm play pos arg :/
cremno commented 2020-03-25 11:13:18 +00:00 (Migrated from github.com)
Copy Link

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.

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.
👍 1
carstene1ns (Migrated from github.com) reviewed 2020-03-25 12:02:07 +00:00
binding-mri/binding-util.h
@ -230,0 +230,4 @@
case RUBY_T_NIL :
*out = 0;
break;
carstene1ns (Migrated from github.com) commented 2020-03-25 12:02:07 +00:00
Copy Link

Wrong indentation (use tabs).

Wrong indentation (use tabs).
Cloudef (Migrated from github.com) reviewed 2020-03-25 13:13:22 +00:00
binding-mri/binding-util.h
@ -230,0 +230,4 @@
case RUBY_T_NIL :
*out = 0;
break;
Cloudef (Migrated from github.com) commented 2020-03-25 13:13:22 +00:00
Copy Link

Guess I have to configure my editor to not convert tabs ...

Guess I have to configure my editor to not convert tabs ...
Cloudef commented 2020-03-25 13:15:19 +00:00 (Migrated from github.com)
Copy Link

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

@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 (Migrated from github.com) reviewed 2020-03-25 13:18:36 +00:00
binding-mri/binding-util.h
@ -230,0 +230,4 @@
case RUBY_T_NIL :
*out = 0;
break;
Cloudef (Migrated from github.com) commented 2020-03-25 13:18:36 +00:00
Copy Link

Also would you happen to know what are the exact tab width etc settings for this project.

Also would you happen to know what are the exact tab width etc settings for this project.
Cloudef commented 2020-03-25 13:29:48 +00:00 (Migrated from github.com)
Copy Link

Fixed formatting.

Fixed formatting.
cremno commented 2020-03-25 15:01:56 +00:00 (Migrated from github.com)
Copy Link

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.

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 2020-03-25 15:05:53 +00:00 (Migrated from github.com)
Copy Link

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.

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 2020-03-25 15:07:47 +00:00 (Migrated from github.com)
Copy Link

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.

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 2020-03-25 15:26:53 +00:00 (Migrated from github.com)
Copy Link

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.

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 2020-08-11 14:11:50 +00:00 (Migrated from github.com)
Copy Link

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.

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.
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b Cloudef/ruby-game-compatibility master
git pull origin Cloudef/ruby-game-compatibility

Step 2:

Merge the changes and update on Forgejo.
git checkout master
git merge --no-ff Cloudef/ruby-game-compatibility
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
RGSS accuracy

   
bug

   
compilation

   
discussion

   
documentation

   
duplicate

   
enhancement

   
invalid

   
performance issue

   
port request

   
question

   
ruby incompatibility

   
wontfix
No Label
RGSS accuracy
bug
compilation
discussion
documentation
duplicate
enhancement
invalid
performance issue
port request
question
ruby incompatibility
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: MapleShrine/mkxp#227
No description provided.
Delete Branch "Cloudef/ruby-game-compatibility"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?