Function parameters incorrectly typed as number for classes extending another class

[regnaio]

My apologies if this is related to #200

Inside the https://github.com/kripken/ammo.js repo, I ran:

npx webidl-dts-gen -e -n Ammo -i ammo.idl -o builds/ammo.d.ts

I noticed that for classes that extends another class, function parameter types appear to be incorrectly typed as number:

class btMotionState {
    getWorldTransform(worldTrans: btTransform): void;
    setWorldTransform(worldTrans: btTransform): void;
}
class MotionState extends btMotionState {
    constructor();
    getWorldTransform(worldTrans: number): void; // <--- `number` should be `btTransform`
    setWorldTransform(worldTrans: number): void; // <--- same issue
}

Instead, if we use @milkshakeio/webidl2ts, then we get the correct result:

npx @milkshakeio/webidl2ts -e -n Ammo -i ammo.idl -o builds/ammo.d.ts

class btMotionState {
    getWorldTransform(worldTrans: btTransform): void;
    setWorldTransform(worldTrans: btTransform): void;
}
class MotionState {
    constructor();
    getWorldTransform(worldTrans: btTransform): void;
    setWorldTransform(worldTrans: btTransform): void;
}

I still prefer to use webidl-dts-gen over @milkshakeio/webidl2ts, since it handles enums correctly.

Thank you for all your help!

[isaac-mason]

Thanks for raising this issue! I'll find some time to look at this soon.

[isaac-mason]

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;
    }

[regnaio]

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?

[isaac-mason]

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.