Skip to content
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

Jump to bottom

Rational errors fix #239

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rational errors fix #239

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions binding-mri/binding-util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ rb_float_arg(VALUE arg, double *out, int argPos = 0)
break;

default:
rb_raise(rb_eTypeError, "Argument %d: Expected float", argPos);
rb_raise(rb_eTypeError, "Argument %d: Expected float(got 0x%x)", argPos, rb_type(arg));
}
}

Expand All @@ -243,11 +243,12 @@ rb_int_arg(VALUE arg, int *out, int argPos = 0)
break;

case RUBY_T_FIXNUM :
case RUBY_T_RATIONAL :
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this? FIX2INT() only works with fixnums.

Copy link
Author

@pk-2000 pk-2000 Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes,
the only games that I encountered this problem are some rgss1 games,
that use a plugin to determine the characters position.
float value (X,Y position of the character) & rational value (expected X,Y new value)
The new value is determined from the move direction of the character.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, RGSS3 also supports rational numbers but your change may only appear to work. Could you please try something like Rect.new(Rational(10, 10), 0, 0, 0) and check the value of its x attribute? I can't compile mkxp at the moment and there's no test suite or build bot.

IMHO the type check isn't necessary anyway. *out = NUM2INT(arg); is good enough (and rb_rescue() if the custom error message should be kept). This would add support for complex numbers too: Rect.new(Complex(10, 0), 0, 0, 0).

Copy link
Author

@pk-2000 pk-2000 Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can you give another test?
My Falcon-mkxp has unlocked resolution, so the x result will always deffer, depending on the game resolution & monitor resolution & position of the window.

  • I had tested it against these examples
    https://ruby-doc.org/core-2.5.3/Rational.html
    and the only ones that could not resolve (if I remember correctly) were the examples with half keyword arguments e.g.
    Rational(25, 100).round(1, half: :up) #=> (3/10)

Copy link
Contributor

Choose a reason for hiding this comment

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

X should always be 1. But it can be tested with any other integer attribute:

s = Sprite.new
s.z = Rational(10, 2)
p s.z # => 5

Copy link
Author

Choose a reason for hiding this comment

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

Thank you.
I created 2 events to call the test scripts and both of them
return positive random(?) values with 8 digits(!) instead of returning x=1 & s=5.

*out = FIX2INT(arg);
break;

default:
rb_raise(rb_eTypeError, "Argument %d: Expected fixnum", argPos);
rb_raise(rb_eTypeError, "Argument %d: Expected fixnum(got 0x%x)", argPos, rb_type(arg));
}
}

Expand Down