Fix mruby bindings #197

Open
pulsejet wants to merge 5 commits from pulsejet/mrb into master
9 changed files with 3317 additions and 2490 deletions

View File

@ -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);
if (rgssVer == 1)
Ancurio commented 2018-04-29 09:25:55 +00:00 (Migrated from github.com)
Review

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.
mrb_define_global_const(mrb, "DEBUG", debug);
else if (rgssVer >= 2)
@ -298,7 +298,7 @@ runRMXPScripts(mrb_state *mrb, mrbc_context *ctx)
return;
}
int scriptCount = mrb_ary_len(scriptMrb, scriptArray);
int scriptCount = RARRAY_LEN(scriptArray);
std::string decodeBuffer;
decodeBuffer.resize(0x1000);

View File

@ -71,8 +71,7 @@ static const MrbExcData excData[] =
{ Shutdown, "SystemExit" },
Ancurio commented 2018-04-29 09:26:45 +00:00 (Migrated from github.com)
Review

Why the rename? Is IOError already defined by mruby?

Why the rename? Is `IOError` already defined by mruby?
pulsejet commented 2018-04-29 12:32:43 +00:00 (Migrated from github.com)
Review

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 commented 2018-04-29 12:42:57 +00:00 (Migrated from github.com)
Review

Should I rename this to something better?

Should I rename this to something better?
Ancurio commented 2018-05-05 09:28:45 +00:00 (Migrated from github.com)
Review

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
pulsejet commented 2018-11-25 12:14:12 +00:00 (Migrated from github.com)
Review

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
{ PHYSFS, "PHYSFSError" },
{ SDL, "SDLError" },
{ MKXP, "MKXPError" },
{ IO, "IOError" }
{ MKXP, "MKXPError" }
};
static elementsN(excData);
@ -118,6 +117,7 @@ MrbData::MrbData(mrb_state *mrb)
exc[TypeError] = mrb_class_get(mrb, "TypeError");
exc[ArgumentError] = mrb_class_get(mrb, "ArgumentError");
exc[IOError] = mrb_class_get(mrb, "IOError");
for (size_t i = 0; i < symDataN; ++i)
symbols[symData[i].ind] = mrb_intern_cstr(mrb, symData[i].str);
@ -134,6 +134,7 @@ static const MrbException excToMrbExc[] =
TypeError,
ArgumentError,
IOError,
PHYSFS, /* PHYSFSError */
SDL, /* SDLError */

View File

@ -28,6 +28,7 @@
#include <mruby/data.h>
#include <mruby/variable.h>
#include <mruby/class.h>
#include <mruby/string.h>
#include <stdio.h>
@ -90,6 +91,7 @@ enum MrbException
TypeError,
ArgumentError,
IOError,
MrbExceptionsMax
};
@ -352,11 +354,13 @@ inline mrb_value
objectLoad(mrb_state *mrb, mrb_value self, const mrb_data_type &type)
{
RClass *klass = mrb_class_ptr(self);
char *data;
int data_len;
mrb_get_args(mrb, "s", &data, &data_len);
C *c = C::deserialize(data, data_len);
mrb_value data;
mrb_get_args(mrb, "S", &data);
int data_len = mrb_string_value_len(mrb, data);
C *c = C::deserialize(RSTRING_PTR(data), data_len);
RData *obj = mrb_data_object_alloc(mrb, klass, c, &type);
mrb_value obj_value = mrb_obj_value(obj);

View File

@ -49,7 +49,12 @@ MRB_METHOD(fontInitialize)
mrb_get_args(mrb, "|zi", &name, &size);
Font *f = new Font(name, size);
Font *f;
std::vector<std::string> names;
names.push_back(name);
f = new Font(&names, size);
setPrivateData(self, f, FontType);
@ -86,9 +91,7 @@ MRB_METHOD(fontInitializeCopy)
MRB_METHOD(FontGetName)
{
Font *f = getPrivateData<Font>(mrb, self);
return mrb_str_new_cstr(mrb, f->getName());
return mrb_iv_get(mrb, self, mrb_intern_cstr(mrb, "name"));
}
MRB_METHOD(FontSetName)
@ -98,7 +101,11 @@ MRB_METHOD(FontSetName)
mrb_value name;
mrb_get_args(mrb, "S", &name);
f->setName(RSTRING_PTR(name));
std::vector<std::string> names;
names.push_back(RSTRING_PTR(name));
f->setName(names);
mrb_iv_set(mrb, self, mrb_intern_cstr(mrb, "name"), name);
return name;
}
@ -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 commented 2018-04-29 09:34:13 +00:00 (Migrated from github.com)
Review

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.
MRB_FUNCTION(FontGetDefaultName)
MRB_METHOD(FontGetDefaultName)
{
return mrb_str_new_cstr(mrb, Font::getDefaultName());
return mrb_iv_get(mrb, self, mrb_intern_cstr(mrb, "default_name"));
}
MRB_FUNCTION(FontSetDefaultName)
MRB_METHOD(FontSetDefaultName)
{
mrb_value nameObj;
mrb_get_args(mrb, "S", &nameObj);
Font::setDefaultName(RSTRING_PTR(nameObj));
std::vector<std::string> names;
names.push_back(RSTRING_PTR(nameObj));
Font::setDefaultName(names, shState->fontState());
mrb_iv_set(mrb, self, mrb_intern_cstr(mrb, "default_name"), nameObj);
return nameObj;
}

File diff suppressed because it is too large Load Diff

3275
binding-mruby/module_rpg.xxd Normal file

File diff suppressed because it is too large Load Diff

View File

@ -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");
mrb_define_module_function(mrb, module, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
Ancurio commented 2018-04-29 09:35:03 +00:00 (Migrated from github.com)
Review

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?
pulsejet commented 2018-04-29 12:39:22 +00:00 (Migrated from github.com)
Review

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?
Ancurio commented 2018-05-01 11:35:27 +00:00 (Migrated from github.com)
Review

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.
pulsejet commented 2018-05-05 09:17:09 +00:00 (Migrated from github.com)
Review

@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 commented 2018-05-05 09:31:59 +00:00 (Migrated from github.com)
Review

@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`?
pulsejet commented 2018-05-05 10:25:00 +00:00 (Migrated from github.com)
Review

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:57:29 +00:00 (Migrated from github.com)
Review

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.
mrb_define_module_function(mrb, module, "directory?", fileTestIsDirectory, MRB_ARGS_REQ(1));
mrb_define_module_function(mrb, module, "file?", fileTestIsFile, MRB_ARGS_REQ(1));

View File

@ -172,7 +172,7 @@ MRB_FUNCTION(kernelLoadData)
mrb_get_args(mrb, "z", &filename);
SDL_RWops ops;
GUARD_EXC( shState->fileSystem().openRead(ops, filename); )
GUARD_EXC( shState->fileSystem().openReadRaw(ops, filename); )
mrb_value obj;
try { obj = marshalLoadInt(mrb, &ops); }

View File

@ -473,8 +473,8 @@ read_value(MarshalContext *ctx)
mrb_state *mrb = ctx->mrb;
int8_t type = ctx->readByte();
mrb_value value;
if (mrb->arena_idx > maxArena)
maxArena = mrb->arena_idx;
if (mrb->gc.arena_idx > maxArena)
maxArena = mrb->gc.arena_idx;
Ancurio commented 2018-04-29 09:36:07 +00:00 (Migrated from github.com)
Review

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 :)
int arena = mrb_gc_arena_save(mrb);
@ -676,7 +676,7 @@ static void
write_array(MarshalContext *ctx, mrb_value array)
{
mrb_state *mrb = ctx->mrb;
int len = mrb_ary_len(mrb, array);
int len = RARRAY_LEN(array);
write_fixnum(ctx, len);
int i;
@ -687,8 +687,6 @@ write_array(MarshalContext *ctx, mrb_value array)
}
}
KHASH_DECLARE(ht, mrb_value, mrb_value, 1)
static void
write_hash(MarshalContext *ctx, mrb_value hash)
{
@ -707,7 +705,7 @@ write_hash(MarshalContext *ctx, mrb_value hash)
continue;
write_value(ctx, kh_key(h, k));
write_value(ctx, kh_val(h, k));
write_value(ctx, kh_val(h, k).v);
}
}
@ -732,7 +730,7 @@ write_object(MarshalContext *ctx, mrb_value object)
write_value(ctx, mrb_str_intern(mrb, path));
mrb_value iv_ary = mrb_obj_instance_variables(mrb, object);
int iv_count = mrb_ary_len(mrb, iv_ary);
int iv_count = RARRAY_LEN(iv_ary);
write_fixnum(ctx, iv_count);
int i;