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.

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

Rational errors fix #239

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

Conversation

pk-2000
Copy link

@pk-2000 pk-2000 commented Aug 31, 2021

  • Better debug information on float errors (lines 231 and 251).
  • line 246 fixes ruby rational errors

- Better debug information on float errors (lines 231 and 251).
- line 246 fixes ruby rational errors
@@ -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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants