-
Notifications
You must be signed in to change notification settings - Fork 132
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
Fix mruby bindings #197
base: master
Are you sure you want to change the base?
Fix mruby bindings #197
Conversation
Good to see someone work on this! 👍 |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -117,7 +117,7 @@ static void mrbBindingInit(mrb_state *mrb) | |||
/* Load global constants */ | |||
mrb_define_global_const(mrb, "MKXP", mrb_true_value()); | |||
|
|||
mrb_value debug = rb_bool_new(shState->config().editor.debug); | |||
mrb_value debug = mrb_bool_value(shState->config().editor.debug); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, rb_
is from the MRI binding, so this probably slipped in with PR #191.
binding-mruby/binding-util.cpp
Outdated
@@ -72,7 +72,7 @@ static const MrbExcData excData[] = | |||
{ PHYSFS, "PHYSFSError" }, | |||
{ SDL, "SDLError" }, | |||
{ MKXP, "MKXPError" }, | |||
{ IO, "IOError" } | |||
{ IO, "IOError2" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the rename? Is IOError
already defined by mruby?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. If I remember right, this throws up some error about IOError
already existing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I rename this to something better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this, but tbh I've no idea what's going on here
binding-mruby/font-binding.cpp
Outdated
|
||
return mrb_str_new_cstr(mrb, f->getName()); | ||
/* FIXME: getName method is missing from Font */ | ||
return mrb_str_new_cstr(mrb, "name"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return the saved IV here (see below).
binding-mruby/font-binding.cpp
Outdated
@@ -137,15 +144,19 @@ DEF_KLASS_PROP(Font, mrb_bool, DefaultShadow, "b", bool) | |||
|
|||
MRB_FUNCTION(FontGetDefaultName) | |||
{ | |||
return mrb_str_new_cstr(mrb, Font::getDefaultName()); | |||
/* FIXME: getDefaultName method is missing from Font */ | |||
return mrb_str_new_cstr(mrb, "default_name"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with get/SetName, except that the IV is stored in the Class object.
@@ -596,7 +596,7 @@ fileBindingInit(mrb_state *mrb) | |||
mrb_define_method(mrb, klass, "path", fileGetPath, MRB_ARGS_NONE()); | |||
|
|||
/* FileTest */ | |||
RClass *module = mrb_define_module(mrb, "FileTest"); | |||
RClass *module = mrb_define_module(mrb, "MKXPFileTest"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's IO::File
in mruby while FileTest
is defined in the global namespace? That's weird.
if (mrb->arena_idx > maxArena) | ||
maxArena = mrb->arena_idx; | ||
if (mrb->gc.arena_idx > maxArena) | ||
maxArena = mrb->gc.arena_idx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember the internals of mruby too well so I'll just trust that this does the right thing :)
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. |
@@ -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) | |||
|
|||
MRB_FUNCTION(FontGetDefaultName) | |||
MRB_METHOD(FontGetDefaultName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to change this to MRB_METHOD
for self
to be defined. Not sure what the change really means...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
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, |
Could you please compile mruby without the |
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. |
@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. |
Noo @take-cheeze's mruby-marshal can load only once in the sense that you can't get variables by doing |
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. |
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. |
Sorry I completely forgot about this. Removed the |
The bindings currently provided are really old and incompatible with API changes. mkxp compiles and runs against the latest mruby with these changes.