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

Fix mruby bindings #197

Open
pulsejet wants to merge 5 commits from pulsejet/mrb into master
pull from: pulsejet/mrb
merge into: MapleShrine:master
Conversation 12 Commits 5 Files Changed 9 +3317 -2490
pulsejet commented 2018-04-28 09:16:47 +00:00 (Migrated from github.com)
Copy Link

The bindings currently provided are really old and incompatible with API changes. mkxp compiles and runs against the latest mruby with these changes.

The bindings currently provided are really old and incompatible with API changes. mkxp compiles and runs against the latest mruby with these changes.
carstene1ns commented 2018-04-28 09:21:08 +00:00 (Migrated from github.com)
Copy Link

Good to see someone work on this! 👍

Good to see someone work on this! :+1:
pulsejet commented 2018-04-28 11:03:08 +00:00 (Migrated from github.com)
Copy Link

@Ancurio any reason why the RPG module is defined differently for mruby and MRI? The current mruby module gives me really cryptic errors; just adding the scripts from MRI to my Scripts.rxdata works pretty well. With only minor changes, the game seems to work (sorta), though for some reason no events are being triggered as I interact with them :(

@Ancurio any reason why the RPG module is defined differently for mruby and MRI? The current mruby module gives me really cryptic errors; just adding the scripts from MRI to my `Scripts.rxdata` works pretty well. With only minor changes, the game seems to work (sorta), though for some reason no events are being triggered as I interact with them :(
Ancurio (Migrated from github.com) requested changes 2018-04-29 09:37:54 +00:00
Ancurio (Migrated from github.com) left a comment
Copy Link

Looks good overall; sorry for the state the mruby binding has been in, I haven't used it in a long while and thus neglected maintenance. Would be great if there was an actual use for it besides just simple test scripts.
Thanks for working on it.

Looks good overall; sorry for the state the mruby binding has been in, I haven't used it in a long while and thus neglected maintenance. Would be great if there was an actual use for it besides just simple test scripts. Thanks for working on it.
binding-mruby/binding-mruby.cpp
@ -119,3 +119,3 @@
mrb_value debug = rb_bool_new(shState->config().editor.debug);
mrb_value debug = mrb_bool_value(shState->config().editor.debug);
if (rgssVer == 1)
Ancurio (Migrated from github.com) commented 2018-04-29 09:25:55 +00:00
Copy Link

hm, rb_ is from the MRI binding, so this probably slipped in with PR #191.

hm, `rb_` is from the MRI binding, so this probably slipped in with PR #191.
binding-mruby/binding-util.cpp
@ -71,8 +71,7 @@ static const MrbExcData excData[] =
{ Shutdown, "SystemExit" },
Ancurio (Migrated from github.com) commented 2018-04-29 09:26:45 +00:00
Copy Link

Why the rename? Is IOError already defined by mruby?

Why the rename? Is `IOError` already defined by mruby?
binding-mruby/font-binding.cpp
Ancurio (Migrated from github.com) commented 2018-04-29 09:33:05 +00:00
Copy Link

Return the saved IV here (see below).

Return the saved IV here (see below).
binding-mruby/font-binding.cpp
Ancurio (Migrated from github.com) commented 2018-04-29 09:32:46 +00:00
Copy Link

Compare the MRI binding: We additionally save the passed argument as an IV, and return that in the getter.

Compare the MRI binding: We additionally save the passed argument as an IV, and return that in the getter.
binding-mruby/font-binding.cpp
@ -135,17 +142,21 @@ DEF_KLASS_PROP(Font, mrb_bool, DefaultItalic, "b", bool)
DEF_KLASS_PROP(Font, mrb_bool, DefaultOutline, "b", bool)
DEF_KLASS_PROP(Font, mrb_bool, DefaultShadow, "b", bool)
Ancurio (Migrated from github.com) commented 2018-04-29 09:34:13 +00:00
Copy Link

Same as with get/SetName, except that the IV is stored in the Class object.

Same as with get/SetName, except that the IV is stored in the Class object.
binding-mruby/mrb-ext/file.cpp
@ -598,3 +598,3 @@
/* FileTest */
RClass *module = mrb_define_module(mrb, "FileTest");
RClass *module = mrb_define_module(mrb, "MKXPFileTest");
mrb_define_module_function(mrb, module, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
Ancurio (Migrated from github.com) commented 2018-04-29 09:35:03 +00:00
Copy Link

With this rename the class becomes pointless (scripts expect this standard name, which is also mentioned in the RGSS documentation); why was it necessary? Is there a native implementation now?

With this rename the class becomes pointless (scripts expect this standard name, which is also mentioned in the RGSS documentation); why was it necessary? Is there a native implementation now?
binding-mruby/mrb-ext/kernel.cpp
Ancurio (Migrated from github.com) commented 2018-04-29 09:35:20 +00:00
Copy Link

no spurious whitespace changes pls

no spurious whitespace changes pls
binding-mruby/mrb-ext/marshal.cpp
Ancurio (Migrated from github.com) commented 2018-04-29 09:35:40 +00:00
Copy Link

whitespace change

whitespace change
binding-mruby/mrb-ext/marshal.cpp
@ -477,2 +476,3 @@
maxArena = mrb->arena_idx;
if (mrb->gc.arena_idx > maxArena)
maxArena = mrb->gc.arena_idx;
Ancurio (Migrated from github.com) commented 2018-04-29 09:36:07 +00:00
Copy Link

I don't remember the internals of mruby too well so I'll just trust that this does the right thing :)

I don't remember the internals of mruby too well so I'll just trust that this does the right thing :)
Ancurio commented 2018-04-29 09:55:50 +00:00 (Migrated from github.com)
Copy Link

@pulsejet

any reason why the RPG module is defined differently for mruby and MRI? The current mruby module gives me really cryptic errors; just adding the scripts from MRI to my Scripts.rxdata works pretty well. With only minor changes, the game seems to work (sorta), though for some reason no events are being triggered as I interact with them :(

The mruby RPG module is the pre-compiled bytecode version; if there was a way to do this with MRI I would, but atm I can only feed it plain Ruby source code. You should be able to use your up-to-date version of mruby to update the precompiled bytecode.

@pulsejet > any reason why the RPG module is defined differently for mruby and MRI? The current mruby module gives me really cryptic errors; just adding the scripts from MRI to my Scripts.rxdata works pretty well. With only minor changes, the game seems to work (sorta), though for some reason no events are being triggered as I interact with them :( The mruby RPG module is the pre-compiled bytecode version; if there was a way to do this with MRI I would, but atm I can only feed it plain Ruby source code. You should be able to use your up-to-date version of mruby to update the precompiled bytecode.
pulsejet (Migrated from github.com) reviewed 2018-04-29 12:32:43 +00:00
binding-mruby/binding-util.cpp
@ -71,8 +71,7 @@ static const MrbExcData excData[] =
{ Shutdown, "SystemExit" },
pulsejet (Migrated from github.com) commented 2018-04-29 12:32:43 +00:00
Copy Link

Yeah. If I remember right, this throws up some error about IOError already existing.

Yeah. If I remember right, this throws up some error about `IOError` already existing.
pulsejet (Migrated from github.com) reviewed 2018-04-29 12:39:22 +00:00
binding-mruby/mrb-ext/file.cpp
@ -598,3 +598,3 @@
/* FileTest */
RClass *module = mrb_define_module(mrb, "FileTest");
RClass *module = mrb_define_module(mrb, "MKXPFileTest");
mrb_define_module_function(mrb, module, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
pulsejet (Migrated from github.com) commented 2018-04-29 12:39:22 +00:00
Copy Link

Yup, it is in the mruby-io mrbgem that is included with the default build (check this, for some reason, the documentation doesn't list this). I'm a bit inclined towards keeping this code for a while since I haven't really tested much yet, so just renamed it to MKXPFileTest. What do you think?

Yup, it is in the `mruby-io` mrbgem that is included with the default build (check [this](https://github.com/mruby/mruby/blob/b724e06cf25eee4c05dcc0e3a1a65bb608091cc4/mrbgems/mruby-io/src/file_test.c), for some reason, the documentation doesn't list this). I'm a bit inclined towards keeping this code for a while since I haven't really tested much yet, so just renamed it to `MKXPFileTest`. What do you think?
pulsejet (Migrated from github.com) reviewed 2018-04-29 12:41:12 +00:00
binding-mruby/mrb-ext/kernel.cpp
pulsejet (Migrated from github.com) commented 2018-04-29 12:41:11 +00:00
Copy Link

Sorry about this. Perils of working late night =|

Sorry about this. Perils of working late night =|
pulsejet (Migrated from github.com) reviewed 2018-04-29 12:42:57 +00:00
binding-mruby/binding-util.cpp
@ -71,8 +71,7 @@ static const MrbExcData excData[] =
{ Shutdown, "SystemExit" },
pulsejet (Migrated from github.com) commented 2018-04-29 12:42:57 +00:00
Copy Link

Should I rename this to something better?

Should I rename this to something better?
pulsejet (Migrated from github.com) reviewed 2018-04-29 13:22:55 +00:00
binding-mruby/font-binding.cpp
pulsejet (Migrated from github.com) commented 2018-04-29 13:22:55 +00:00
Copy Link

Done

Done
pulsejet (Migrated from github.com) reviewed 2018-04-29 13:23:41 +00:00
binding-mruby/font-binding.cpp
pulsejet (Migrated from github.com) commented 2018-04-29 13:23:40 +00:00
Copy Link

Needed to change this to MRB_METHOD for self to be defined. Not sure what the change really means...

Needed to change this to `MRB_METHOD` for `self` to be defined. Not sure what the change really means...
pulsejet commented 2018-04-29 13:48:28 +00:00 (Migrated from github.com)
Copy Link

@Ancurio

The mruby RPG module is the pre-compiled bytecode version; if there was a way to do this with MRI I would, but atm I can only feed it plain Ruby source code.

Would there be any realistic advantage of doing this?

Fixed up the requested changes and updated the RPG module. Decided to move the bytecode to a separate xxd file the way the mri bindings do; is there any argument against this?

@Ancurio > The mruby RPG module is the pre-compiled bytecode version; if there was a way to do this with MRI I would, but atm I can only feed it plain Ruby source code. Would there be any realistic advantage of doing this? Fixed up the requested changes and updated the RPG module. Decided to move the bytecode to a separate `xxd` file the way the mri bindings do; is there any argument against this?
Ancurio (Migrated from github.com) reviewed 2018-05-01 11:35:27 +00:00
binding-mruby/mrb-ext/file.cpp
@ -598,3 +598,3 @@
/* FileTest */
RClass *module = mrb_define_module(mrb, "FileTest");
RClass *module = mrb_define_module(mrb, "MKXPFileTest");
mrb_define_module_function(mrb, module, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
Ancurio (Migrated from github.com) commented 2018-05-01 11:35:27 +00:00
Copy Link

Renaming the Module defeats the entire purpose of having it; code expecting FileTest functions will not work. Could you check whether the mruby-io gem covers all of the functions mentioned in the RGSS1 documentation? Either we require the gem to be disabled for mkxp, or drop the mkxp code in favor of the gem.

Renaming the Module defeats the entire purpose of having it; code expecting `FileTest` functions will not work. Could you check whether the `mruby-io` gem covers all of the functions mentioned in the RGSS1 documentation? Either we require the gem to be disabled for mkxp, or drop the mkxp code in favor of the gem.
Ancurio (Migrated from github.com) reviewed 2018-05-01 11:36:56 +00:00
binding-mruby/font-binding.cpp
Ancurio (Migrated from github.com) commented 2018-05-01 11:36:56 +00:00
Copy Link

It's what you said; only a method has an object to operate on (self), functions don't. The FUNCTION macro was to avoid having to (void) the self argument every time in module functions.

It's what you said; only a method has an object to operate on (self), functions don't. The `FUNCTION` macro was to avoid having to `(void)` the self argument every time in module functions.
Ancurio commented 2018-05-01 11:39:55 +00:00 (Migrated from github.com)
Copy Link

Would there be any realistic advantage of doing this?

Maybe it just irks me to parse the same source code every time on startup; but you're right, it might be less of a headache to always just parse source instead having to compile against a possibly changing bytecode-specification (like with mruby); and I believe there might be far heavier tasks going on at startup.

Ideally though, the bytecode compilation would be integrated in mkxp's build process,

> Would there be any realistic advantage of doing this? Maybe it just irks me to parse the same source code every time on startup; but you're right, it might be less of a headache to always just parse source instead having to compile against a possibly changing bytecode-specification (like with mruby); and I believe there might be far heavier tasks going on at startup. Ideally though, the bytecode compilation would be integrated in mkxp's build process,
Ancurio commented 2018-05-01 11:49:45 +00:00 (Migrated from github.com)
Copy Link

Could you please compile mruby without the io gem and only keep the fixes required to get that running? We can move eventual changes from replacing my custom code with the gem into a separate issue/PR.

Could you please compile mruby without the `io` gem and only keep the fixes required to get that running? We can move eventual changes from replacing my custom code with the gem into a separate issue/PR.
pulsejet commented 2018-05-05 09:08:43 +00:00 (Migrated from github.com)
Copy Link

Sorry I didn't see this. Going out of town for now; I'll fix this when I get back. @Ancurio do we also want to make the change to to mruby-marshal? It seems to be much more complete and saving/loading works with minor changes (you can dump/load only once) unlike the current marshal class. One downside is that it depends on onig.

Sorry I didn't see this. Going out of town for now; I'll fix this when I get back. @Ancurio do we also want to make the change to to [mruby-marshal](https://github.com/take-cheeze/mruby-marshal)? It seems to be much more complete and saving/loading works with minor changes (you can dump/load only once) unlike the current marshal class. One downside is that it depends on onig.
pulsejet (Migrated from github.com) reviewed 2018-05-05 09:17:09 +00:00
binding-mruby/mrb-ext/file.cpp
@ -598,3 +598,3 @@
/* FileTest */
RClass *module = mrb_define_module(mrb, "FileTest");
RClass *module = mrb_define_module(mrb, "MKXPFileTest");
mrb_define_module_function(mrb, module, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
pulsejet (Migrated from github.com) commented 2018-05-05 09:17:09 +00:00
Copy Link

@Ancurio just checked, it does implement everything we want (and much more) for FileTest and nothing for File (so no overlapping symbols). Actually I'm strongly against disabling mruby-io, since being a core mrbgem, this will definitely break something else (mruby-marshal for a start)

@Ancurio just checked, it does implement everything we want (and much more) for `FileTest` and nothing for `File` (so no overlapping symbols). Actually I'm strongly against disabling `mruby-io`, since being a core mrbgem, this will definitely break something else (mruby-marshal for a start)
Ancurio (Migrated from github.com) reviewed 2018-05-05 09:28:45 +00:00
binding-mruby/binding-util.cpp
@ -71,8 +71,7 @@ static const MrbExcData excData[] =
{ Shutdown, "SystemExit" },
Ancurio (Migrated from github.com) commented 2018-05-05 09:28:45 +00:00
Copy Link

This is from the io-gem too, right. Just delete this entry in the array, and query the existing error class like done here: https://github.com/Ancurio/mkxp/blob/master/binding-mruby/binding-util.cpp#L120

This is from the io-gem too, right. Just delete this entry in the array, and query the existing error class like done here: https://github.com/Ancurio/mkxp/blob/master/binding-mruby/binding-util.cpp#L120
Ancurio (Migrated from github.com) reviewed 2018-05-05 09:31:59 +00:00
binding-mruby/mrb-ext/file.cpp
@ -598,3 +598,3 @@
/* FileTest */
RClass *module = mrb_define_module(mrb, "FileTest");
RClass *module = mrb_define_module(mrb, "MKXPFileTest");
mrb_define_module_function(mrb, module, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
Ancurio (Migrated from github.com) commented 2018-05-05 09:31:59 +00:00
Copy Link

@pulsejet Oh I'm sorry, I just finally understood why you renamed the module; my brain must have been lagging. The rename is fine (although I'd like to delete my code as soon as possible), with a comment about why the old classes exist.
I'm still confused about File though; why did you not have to rename that to MKXPFile?

@pulsejet Oh I'm sorry, I just finally understood why you renamed the module; my brain must have been lagging. The rename is fine (although I'd like to delete my code as soon as possible), with a comment about why the old classes exist. I'm still confused about `File` though; why did you not have to rename that to `MKXPFile`?
Ancurio commented 2018-05-05 09:34:35 +00:00 (Migrated from github.com)
Copy Link

@pulsejet Wait, my marshal code can only dump / load once? That's not supposed to happen.. I think. The Marshal gem by @take-cheeze is not merged into mruby, is it? I would consider switching to it; the more code being shared, the better, especially if it's more complete.
Do you know the status of regular expressions in mruby? I also remember having quite a few problems with that in the past.

@pulsejet Wait, my marshal code can only dump / load once? That's not supposed to happen.. I think. The Marshal gem by @take-cheeze is not merged into mruby, is it? I would consider switching to it; the more code being shared, the better, especially if it's more complete. Do you know the status of regular expressions in mruby? I also remember having quite a few problems with that in the past.
pulsejet commented 2018-05-05 10:21:44 +00:00 (Migrated from github.com)
Copy Link

Noo @take-cheeze's mruby-marshal can load only once in the sense that you can't get variables by doing variable_name = Marshal.load(file) repeatedly, as it will set the entire contents of the file to variable_name and for dump any repeated calls are ignored. That part isn't merged yet. Sorry for the confusion.
For Regex, I'm using mruby-onig-regexp, but I've also tried mruby-regexp-pcre and both seem to work fine, at least as far as I tested.

Noo @take-cheeze's mruby-marshal can load only once in the sense that you can't get variables by doing `variable_name = Marshal.load(file)` repeatedly, as it will set the entire contents of the file to `variable_name` and for dump any repeated calls are ignored. That part isn't merged yet. Sorry for the confusion. For Regex, I'm using [mruby-onig-regexp](https://github.com/mattn/mruby-onig-regexp), but I've also tried [mruby-regexp-pcre](https://github.com/iij/mruby-regexp-pcre) and both seem to work fine, at least as far as I tested.
pulsejet (Migrated from github.com) reviewed 2018-05-05 10:25:00 +00:00
binding-mruby/mrb-ext/file.cpp
@ -598,3 +598,3 @@
/* FileTest */
RClass *module = mrb_define_module(mrb, "FileTest");
RClass *module = mrb_define_module(mrb, "MKXPFileTest");
mrb_define_module_function(mrb, module, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
pulsejet (Migrated from github.com) commented 2018-05-05 10:25:00 +00:00
Copy Link

Weirdly, the File class of the mruby-io is completely different, while FileTest is exactly the same, so we need File (though I've not tried removing it). Not really sure about anything here, but one thing I can confirm is that this code works :P

Weirdly, the `File` class of the `mruby-io` is completely different, while `FileTest` is exactly the same, so we need `File` (though I've not tried removing it). Not really sure about *anything* here, but one thing I can confirm is that `this code works` :P
Ancurio commented 2018-05-05 10:54:08 +00:00 (Migrated from github.com)
Copy Link

I wonder if @take-cheeze 's implementation can be configured for different RegExp implementations. Also on the dumping / loading only working once, that's a different behavior from MRI then, isn't it? I wonder if that is intentional or a bug.

I wonder if @take-cheeze 's implementation can be configured for different RegExp implementations. Also on the dumping / loading only working once, that's a different behavior from MRI then, isn't it? I wonder if that is intentional or a bug.
Ancurio (Migrated from github.com) reviewed 2018-05-05 10:57:29 +00:00
binding-mruby/mrb-ext/file.cpp
@ -598,3 +598,3 @@
/* FileTest */
RClass *module = mrb_define_module(mrb, "FileTest");
RClass *module = mrb_define_module(mrb, "MKXPFileTest");
mrb_define_module_function(mrb, module, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
Ancurio (Migrated from github.com) commented 2018-05-05 10:57:29 +00:00
Copy Link

b724e06cf2/mrbgems/mruby-io/src/file.c (L395)

b724e06cf2/mrbgems/mruby-io/src/file_test.c (L365)

So it's IO::File in mruby while FileTest is defined in the global namespace? That's weird.

https://github.com/mruby/mruby/blob/b724e06cf25eee4c05dcc0e3a1a65bb608091cc4/mrbgems/mruby-io/src/file.c#L395 https://github.com/mruby/mruby/blob/b724e06cf25eee4c05dcc0e3a1a65bb608091cc4/mrbgems/mruby-io/src/file_test.c#L365 So it's `IO::File` in mruby while `FileTest` is defined in the global namespace? That's weird.
pulsejet commented 2018-05-05 11:00:35 +00:00 (Migrated from github.com)
Copy Link

Might be intentional, not sure. How is the behavior of MRI supposed to be implemented anyway? As for regex, I think using onig might be recommendable anyway, since the PCRE implementation says that you might want to use onig if you want better compatibility with MRI.

Might be intentional, not sure. How is the behavior of MRI supposed to be implemented anyway? As for regex, I think using onig might be recommendable anyway, since the PCRE implementation says that you might want to use onig if you want better compatibility with MRI.
pulsejet commented 2018-11-25 12:04:59 +00:00 (Migrated from github.com)
Copy Link

Sorry I completely forgot about this. Removed the IOError2 thing (don't even remember much now) and right now this just works with mruby 1.4.1 compiled with the default gems (which includes io, btw), and the default game seems to run fine (with the exception of save/load, of course). @Ancurio could you please go over this quickly one more time?

Sorry I completely forgot about this. Removed the `IOError2` thing (don't even remember much now) and right now this _just works_ with mruby 1.4.1 compiled with the default gems (which includes `io`, btw), and the default game seems to run fine (with the exception of save/load, of course). @Ancurio could you please go over this quickly one more time?
pulsejet (Migrated from github.com) reviewed 2018-11-25 12:14:12 +00:00
binding-mruby/binding-util.cpp
@ -71,8 +71,7 @@ static const MrbExcData excData[] =
{ Shutdown, "SystemExit" },
pulsejet (Migrated from github.com) commented 2018-11-25 12:14:12 +00:00
Copy Link

Did this, but tbh I've no idea what's going on here

Did this, but tbh I've no idea what's going on here
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 pulsejet/mrb master
git pull origin pulsejet/mrb

Step 2:

Merge the changes and update on Forgejo.
git checkout master
git merge --no-ff pulsejet/mrb
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Ancurio
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#197
No description provided.
Delete Branch "pulsejet/mrb"

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?