Open regnaio opened 1 month ago
Thanks for raising this issue! I'll find some time to look at this soon.
This is related to some logic for generating correct types for javascript subclasses of c++ classes using JSImplementation (see: https://emscripten.org/docs/porting/connecting_cpp_and_javascript/WebIDL-Binder.html#sub-classing-c-base-classes-in-javascript-jsimplementation).
For example, in ammo.js Ammo.ConcreteContactResultCallback is used to get collision results.
Here is it's webidl:
[Prefix="btCollisionWorld::"]
interface ContactResultCallback {
float addSingleResult([Ref] btManifoldPoint cp, [Const] btCollisionObjectWrapper colObj0Wrap, long partId0, long index0, [Const] btCollisionObjectWrapper colObj1Wrap, long partId1, long index1);
};
[JSImplementation="ContactResultCallback"]
interface ConcreteContactResultCallback {
void ConcreteContactResultCallback();
float addSingleResult([Ref] btManifoldPoint cp, [Const] btCollisionObjectWrapper colObj0Wrap, long partId0, long index0, [Const] btCollisionObjectWrapper colObj1Wrap, long partId1, long index1);
};
With emscripten webidl bindings, the javascript implementation of addSingleResult
isn't called with wrapped classes but with numeric pointers. Because of this, in emscripten mode webidl-dts-gen will generate types accordingly:
class ContactResultCallback {
addSingleResult(cp: btManifoldPoint, colObj0Wrap: btCollisionObjectWrapper, partId0: number, index0: number, colObj1Wrap: btCollisionObjectWrapper, partId1: number, index1: number): number;
}
class ConcreteContactResultCallback extends ContactResultCallback {
constructor();
addSingleResult(cp: number, colObj0Wrap: number, partId0: number, index0: number, colObj1Wrap: number, partId1: number, index1: number): number;
}
My understanding was that classes that use JSImplementation are used for callbacks, and the methods are generally not called from the javascript side. Classes using JSImplementation need to have all methods implemented in javascript.
Is the MotionState class in ammo.js something that is meant to be called in javascript?
I also noted there is btDefaultMotionState
that extends btMotionState
and has expected argument types:
class btMotionState {
getWorldTransform(worldTrans: btTransform): void;
setWorldTransform(worldTrans: btTransform): void;
}
class MotionState extends btMotionState {
constructor();
getWorldTransform(worldTrans: number): void;
setWorldTransform(worldTrans: number): void;
}
class btDefaultMotionState extends btMotionState {
constructor(startTrans?: btTransform, centerOfMassOffset?: btTransform);
get_m_graphicsWorldTrans(): btTransform;
set_m_graphicsWorldTrans(m_graphicsWorldTrans: btTransform): void;
m_graphicsWorldTrans: btTransform;
}
Thank you so much for your help, @isaac-mason 😄
Is the MotionState class in ammo.js something that is meant to be called in javascript?
It is. Both new btDefaultMotionState(transform)
and const motionState = btRigidBody.getMotionState()
are often used.
I also noted there is btDefaultMotionState that extends btMotionState and has expected argument types:
Ah, interesting point, you're right. So it's not guaranteed that an extending class will have incorrect types
My understanding was that classes that use JSImplementation are used for callbacks, and the methods are generally not called from the javascript side. Classes using JSImplementation need to have all methods implemented in javascript.
Do you think this means we need to make changes on the ammo.js side? Perhaps the IDL file or deeper in the Bullet C++ source?
No worries @regnaio!
Ah, interesting point, you're right. So it's not guaranteed that an extending class will have incorrect types
Only interfaces in idl that use JSImplementation will have numeric argument types to match what emscripten generates. Other types of inheritance aren't affected by this logic, that's right.
Do you think this means we need to make changes on the ammo.js side? Perhaps the IDL file or deeper in the Bullet C++ source?
btDefaultMotionState
is correctly typed, and btRigidBody.getMotionState
returns btMotionState
which is also correctly typed, so for using them no changes are be required.
I'm not familiar with the MotionState
javascript sub-class / JSImplementation class used, so I can't speak to that.
My apologies if this is related to https://github.com/pmndrs/webidl-dts-gen/issues/200
Inside the https://github.com/kripken/ammo.js repo, I ran:
I noticed that for classes that
extends
another class, function parameter types appear to be incorrectly typed asnumber
:Instead, if we use
@milkshakeio/webidl2ts
, then we get the correct result:I still prefer to use
webidl-dts-gen
over@milkshakeio/webidl2ts
, since it handlesenum
s correctly.Thank you for all your help!