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 +28 -19
6 changed files with 28 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 39ac6aecf0 - Show all commits

4
binding-mruby/binding-mruby.cpp
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
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.
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);

2
binding-mruby/binding-util.cpp
View File

@ -72,7 +72,7 @@ static const MrbExcData excData[] =
{ PHYSFS, "PHYSFSError" },
{ SDL, "SDLError" },
{ MKXP, "MKXPError" },
{ IO, "IOError" }
{ IO, "IOError2" }
};
static elementsN(excData);

25
binding-mruby/font-binding.cpp
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,8 @@ MRB_METHOD(fontInitializeCopy)
MRB_METHOD(FontGetName)
{
Font *f = getPrivateData<Font>(mrb, self);
return mrb_str_new_cstr(mrb, f->getName());
/* FIXME: getName method is missing from Font */
return mrb_str_new_cstr(mrb, "name");
}
MRB_METHOD(FontSetName)
@ -98,7 +102,10 @@ 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);
return name;
}
@ -137,7 +144,8 @@ 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");
}
MRB_FUNCTION(FontSetDefaultName)
@ -145,7 +153,10 @@ MRB_FUNCTION(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());
return nameObj;
}

2
binding-mruby/mrb-ext/file.cpp
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
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?
pulsejet commented 2018-04-29 12:39:22 +00:00 (Migrated from github.com)
Review
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?
Ancurio commented 2018-05-01 11:35:27 +00:00 (Migrated from github.com)
Review
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.
pulsejet commented 2018-05-05 09:17:09 +00:00 (Migrated from github.com)
Review
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 commented 2018-05-05 09:31:59 +00:00 (Migrated from github.com)
Review
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`?
pulsejet commented 2018-05-05 10:25:00 +00:00 (Migrated from github.com)
Review
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:57:29 +00:00 (Migrated from github.com)
Review
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.
mrb_define_module_function(mrb, module, "directory?", fileTestIsDirectory, MRB_ARGS_REQ(1));
mrb_define_module_function(mrb, module, "file?", fileTestIsFile, MRB_ARGS_REQ(1));

2
binding-mruby/mrb-ext/kernel.cpp
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); }

12
binding-mruby/mrb-ext/marshal.cpp
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
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 :)
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;