Skip to content

Fix mruby bindings #197

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.

Sign up for GitHub

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

Open
pulsejet wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix mruby bindings #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
39ac6ae
Fix up build with mruby binding
pulsejet Apr 28, 2018
8b21699
Fix segmentation fault while passing data in mruby binding
pulsejet Apr 28, 2018
a996d36
Fix FontGetName and FontGetDefaultName in mruby binding
pulsejet Apr 29, 2018
b90aa74
Update RPG Module for mruby bindings
pulsejet Apr 29, 2018
6fccac8
Get rid of IOError2
pulsejet Nov 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions binding-mruby/binding-mruby.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Copy Markdown
Owner

@Ancurio Ancurio Apr 29, 2018

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.

if (rgssVer == 1)
mrb_define_global_const(mrb, "DEBUG", debug);
else if (rgssVer >= 2)
Expand Down Expand Up @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions binding-mruby/binding-util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ static const MrbExcData excData[] =
{ Shutdown, "SystemExit" },
{ PHYSFS, "PHYSFSError" },
{ SDL, "SDLError" },
{ MKXP, "MKXPError" },
{ IO, "IOError" }
{ MKXP, "MKXPError" }
};

static elementsN(excData);
Expand Down Expand Up @@ -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);
Expand All @@ -134,6 +134,7 @@ static const MrbException excToMrbExc[] =

TypeError,
ArgumentError,
IOError,

PHYSFS, /* PHYSFSError */
SDL, /* SDLError */
Expand Down
12 changes: 8 additions & 4 deletions binding-mruby/binding-util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <mruby/data.h>
#include <mruby/variable.h>
#include <mruby/class.h>
#include <mruby/string.h>

#include <stdio.h>

Expand Down Expand Up @@ -90,6 +91,7 @@ enum MrbException

TypeError,
ArgumentError,
IOError,

MrbExceptionsMax
};
Expand Down Expand Up @@ -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);
Expand Down
29 changes: 20 additions & 9 deletions binding-mruby/font-binding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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)
Expand All @@ -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);
Comment thread
pulsejet marked this conversation as resolved.
mrb_iv_set(mrb, self, mrb_intern_cstr(mrb, "name"), name);

return name;
}
Expand Down Expand Up @@ -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)
Copy link
Copy Markdown
Author

@pulsejet pulsejet Apr 29, 2018 •
edited
Loading

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

Copy link
Copy Markdown
Owner

@Ancurio Ancurio May 1, 2018

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.

{
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;
}
Expand Down
2,466 changes: 2 additions & 2,464 deletions binding-mruby/module_rpg.c
View file
Open in desktop

Large diffs are not rendered by default.

3,275 changes: 3,275 additions & 0 deletions binding-mruby/module_rpg.xxd
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion binding-mruby/mrb-ext/file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Copy Markdown
Owner

@Ancurio Ancurio Apr 29, 2018

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?

Copy link
Copy Markdown
Author

@pulsejet pulsejet Apr 29, 2018

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?

Copy link
Copy Markdown
Owner

@Ancurio Ancurio May 1, 2018

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.

Copy link
Copy Markdown
Author

@pulsejet pulsejet May 5, 2018

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)

Copy link
Copy Markdown
Owner

@Ancurio Ancurio May 5, 2018

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?

Copy link
Copy Markdown
Author

@pulsejet pulsejet May 5, 2018

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

Copy link
Copy Markdown
Owner

@Ancurio Ancurio May 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, "exist?", fileTestDoesExist, MRB_ARGS_REQ(1));
mrb_define_module_function(mrb, module, "directory?", fileTestIsDirectory, MRB_ARGS_REQ(1));
mrb_define_module_function(mrb, module, "file?", fileTestIsFile, MRB_ARGS_REQ(1));
Expand Down
2 changes: 1 addition & 1 deletion binding-mruby/mrb-ext/kernel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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); }
Expand Down
12 changes: 5 additions & 7 deletions binding-mruby/mrb-ext/marshal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Copy Markdown
Owner

@Ancurio Ancurio Apr 29, 2018

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 :)


int arena = mrb_gc_arena_save(mrb);

Expand Down Expand Up @@ -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;
Expand All @@ -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)
{
Expand All @@ -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);
}
}

Expand All @@ -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;
Expand Down