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

[Bug]: QuickJS To Juce conversion : integer are forced to float64 #1513

Open
1 task done
benkuper opened this issue Feb 25, 2025 · 8 comments
Open
1 task done

[Bug]: QuickJS To Juce conversion : integer are forced to float64 #1513

benkuper opened this issue Feb 25, 2025 · 8 comments

Comments

@benkuper
Copy link

Detailed steps on how to reproduce the bug

When calling a function from QuickJS engine to C++, the quickJSToJuce function just check that it's a number and casts it to a float64, losing the integer information (which is crucial when for instance sending OSC messages afterwards and needing to keep the value type all the way).

What is the expected behaviour?

Better check on the type of the JSValue when converting and keeping as much as possible the original type throughout the chain

Operating systems

Windows, macOS, Linux

What versions of the operating systems?

All of them

Architectures

x86_64, Arm64/aarch64

Stacktrace

Plug-in formats (if applicable)

No response

Plug-in host applications (DAWs) (if applicable)

No response

Testing on the develop branch

The bug is present on the develop branch

Code of Conduct

  • I agree to follow the Code of Conduct
@reuk
Copy link
Member

reuk commented Feb 26, 2025

Is this technically feasible? I thought that JS only supported double as a numeric type, so it might be that some large integer values simply cannot be represented in JS.

@benkuper
Copy link
Author

benkuper commented Feb 26, 2025

#1514 this at least works in my use cases whereas the current implementation broke all my software on this matter

@szarvas
Copy link
Member

szarvas commented Mar 12, 2025

I looked at your PR and still think it's justified that we treat Javascript numbers as doubles. The Javascript specification itself treats them as such, so your proposal amounts to exposing QuickJS implementation details. In order to do this, it needs to modify the QuickJS implementation itself.

Moreover using QuickJS is kind of an implementation detail already, as this is entirely hidden from the public JUCE API.

Your code would only have an effect for numbers in the int32 range, which is stored exactly in doubles. So you could get the information you need by a function something like:

bool isInt (double v)
{
    return isExactlyEqual (v, std::round (v));
}

This is just a sketch, but hopefully communicates the idea. This would even have the advantage of working beyond the int32 range and up to +/- 2^53.

@benkuper
Copy link
Author

Thank you for your answer !
I kindly disagree :)
I understand your point, but this data type loss cannot be recovered with a simple integer check. A float that is equal to 1 is not an integer.

My main concern is that without this mod, there is no way to force an integer type from script to c++, and this is the only affected type. We could even say imho that Int and float are as different as float and strings.

It could at least be a compilation option in the juce_JavaScript module, so people choose the behaviour they want but would still be available for people wanting to preserve int type in function calls (and I really hope I'm not the only one ahah, at least all my users are interested in this)

@benkuper
Copy link
Author

Also, I can't find any information on isExactlyEqual...

@szarvas
Copy link
Member

szarvas commented Mar 12, 2025

I understand your concern, but you have to see my perspective here is that of a Javascript language lawyer. I think what you're suggesting is incomplete (works only for int32), brittle (depends on a QuickJS implementation detail), and even requires a custom modification of that implementation detail.

A float that is equal to 1 is not an integer

Javascript has no float or integer type, only Number 1. I'm assuming we aren't discussing BigNum.

there is no way to force an integer type from script to c++

You could use something like callMyNativeFunction (42, "int")

We could even say imho that Int and float are as different as float and strings.

I disagree. Javascript does have a String type. 2

Also, I can't find any information on isExactlyEqual...

I misremembered it. It's exactlyEqual in juce_MathFunctions.h.

Footnotes

  1. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Data_structures#number_type

  2. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Data_structures#string_type

@benkuper
Copy link
Author

I get that... I'm still interested in finding a way to do that properly, I can't ask all my users to change their script for that, and the previous engine was working like that.

@szarvas
Copy link
Member

szarvas commented Mar 12, 2025

I don't think there is any innate integerness to numbers in JS, so what we must be seeing here is QuickJS doing an optimisation. If -std::numeric_limits<int>::min() <= number && number <= -std::numeric_limits<int>::max() and the integral part of the number equals to the number itself (see std::modf), then you can convert it to an int. I leave figuring out the exact details to you.

If you were to fork JUCE you would put this in tryQuickJSToJuce as a separate branch inside if (JS_IsNumber (ptr.value)). I think this would be a better option than depending on the implementation details of QuickJS, but I don't agree with adding this conversion as a general change for all JUCE users.

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

No branches or pull requests

3 participants