Typescript: Missing Method from VehicleContraintCallbacksJS

[DennisSmolek]

In the example for a Vehicle to setup callbacks the final step is to call callbacks.SetVehicleConstraint(constraint)

This get's flagged by the typescript compiler because it doesn't exist on VehicleConstraintCallbacksJS

However, it DOES exist on VehicleConstraintCallbacksEm which I'm not sure what it is.

Tagging @isaac-mason as he seems to be the Typescript guru 😎

Existing Types.d.ts:

class VehicleConstraintCallbacksEm {
        SetVehicleConstraint(inConstraint: VehicleConstraint): void;
    }
class VehicleConstraintCallbacksJS {
      constructor();
      GetCombinedFriction(inWheelIndex: number, inTireFrictionDirection: ETireFrictionDirection, inTireFriction: number, inBody2: Body, inSubShapeID2: SubShapeID): number;
      OnPreStepCallback(inVehicle: VehicleConstraint, inDeltaTime: number, inPhysicsSystem: PhysicsSystem): void;
      OnPostCollideCallback(inVehicle: VehicleConstraint, inDeltaTime: number, inPhysicsSystem: PhysicsSystem): void;
      OnPostStepCallback(inVehicle: VehicleConstraint, inDeltaTime: number, inPhysicsSystem: PhysicsSystem): void;
  }

[jrouwe]

@isaac-mason I think this is a problem with webidl-dts-gen:

interface VehicleConstraintCallbacksEm {
    void SetVehicleConstraint([Ref] VehicleConstraint inConstraint);
};

[JSImplementation="VehicleConstraintCallbacksEm"]
interface VehicleConstraintCallbacksJS {
    void VehicleConstraintCallbacksJS();
    /// ...
};

should result in types.d.ts as:

class VehicleConstraintCallbacksEm {
    SetVehicleConstraint(inConstraint: VehicleConstraint): void;
}
class VehicleConstraintCallbacksJS extends VehicleConstraintCallbacksEm {
    constructor();
    /// ...
}

(note the added extends VehicleConstraintCallbacksEm)

Adding to JoltJS.idl:

VehicleConstraintCallbacksJS implements VehicleConstraintCallbacksEm;

will fix the generated .ts file but doesn't work for all JSImplementation classes as e.g. it will complain:

glue.cpp:201:12: error: cannot initialize return object of type 'TransformedShape *' with an rvalue of type 'int'
  201 |     return EM_ASM_INT({
      |            ^~~~~~~~~~~~
  202 |       var self = Module['getCache'](Module['RayCastBodyCollectorJS'])[$0];
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  203 |       if (!self.hasOwnProperty('GetContext')) throw 'a JSImplementation must implement all functions, you forgot RayCastBodyCollectorJS::GetContext.';
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  204 |       return self['GetContext']();
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  205 |     }, (ptrdiff_t)this);
      |     ~~~~~~~~~~~~~~~~~~~

Which seems to suggest it is now also creating callback bindings for the base class functions, which is not what we want. https://github.com/kripken/ammo.js/blob/main/ammo.idl also doesn't use implements in combination with JSImplementation.

I found an interesting workaround, by adding:

//VehicleConstraintCallbacksJS implements VehicleConstraintCallbacksEm;

webidl-dts-gen will generate the extends correctly while the line doesn't affect the webidl_binder.py script as the line is commented out (which I guess we should treat as another bug in webidl-dts-gen as it should not parse commented out code).

[isaac-mason]

I'll create issues for these in webidl-dts-gen, thanks for the heads up!

[isaac-mason]

https://github.com/pmndrs/webidl-dts-gen/issues/199

https://github.com/pmndrs/webidl-dts-gen/issues/200

[isaac-mason]

I've pushed fixes for these issues now, but I need to do some more testing before releasing them. Hoping I can get them out in the next day or so 🙂

[jrouwe]

Thanks!

[isaac-mason]

I've released webidl-dts-gen 1.10.0 with these changes:

• fix(emscripten): don't apply emscripten inheritance fix for commented inheritance statements
• feat(emscripten): generate types with inheritance for emscripten JSImplementation interfaces

Will create a PR bumping JoltPhysics.js shortly 🙂

[isaac-mason]

Oops - webidl-dts-gen may need another fix. I didn't account for inheritance from both JSImplementation and an implements webidl statement

With the latest version of webidl-dts-gen, this webidl:

interface PathConstraintPath {
    boolean IsLooping();
    void SetIsLooping(boolean inIsLooping);
    unsigned long GetRefCount();
    void AddRef();
    void Release();
};

interface PathConstraintPathEm {
};

PathConstraintPathJS implements PathConstraintPath;

[JSImplementation="PathConstraintPathEm"]
interface PathConstraintPathJS {
    [Const] void PathConstraintPathJS();
    [Const] float GetPathMaxFraction();
    [Const] float GetClosestPoint([Const] Vec3 inPosition, float inFractionHint);
    [Const] void GetPointOnPath(float inFraction, Vec3 outPathPosition, Vec3 outPathTangent, Vec3 outPathNormal, Vec3 outPathBinormal);
};

Results in these types:

    class PathConstraintPath {
        IsLooping(): boolean;
        SetIsLooping(inIsLooping: boolean): void;
        GetRefCount(): number;
        AddRef(): void;
        Release(): void;
    }
    class PathConstraintPathEm {
    }
    class PathConstraintPathJS extends PathConstraintPath, PathConstraintPathEm {
        constructor();
        GetPathMaxFraction(): number;
        GetClosestPoint(inPosition: number, inFractionHint: number): number;
        GetPointOnPath(inFraction: number, outPathPosition: number, outPathTangent: number, outPathNormal: number, outPathBinormal: number): void;
    }

Which is invalid syntax, as javascript/typescript classes can only extend one class.

[isaac-mason]

For the moment I'm thinking of changing webidl-dts-gen to only add extends for JSImplementation if there's not already an implements statement.

I also wonder if making this adjustment to the idl for PathConstraint makes sense:

- PathConstraintPathJS implements PathConstraintPath;
+ PathConstraintPathEm implements PathConstraintPath;

    class PathConstraintPath {
        IsLooping(): boolean;
        SetIsLooping(inIsLooping: boolean): void;
        GetRefCount(): number;
        AddRef(): void;
        Release(): void;
    }
    class PathConstraintPathEm extends PathConstraintPath {
    }
    class PathConstraintPathJS extends PathConstraintPathEm {
        constructor();
        GetPathMaxFraction(): number;
        GetClosestPoint(inPosition: number, inFractionHint: number): number;
        GetPointOnPath(inFraction: number, outPathPosition: number, outPathTangent: number, outPathNormal: number, outPathBinormal: number): void;
    }

[isaac-mason]

I released webidl-dts-gen 1.11.0, now implements takes precedence over JSImplementation for the what the class extends. I'm keen to hear what others think the behaviour should be 🙂

Also created a PR: https://github.com/jrouwe/JoltPhysics.js/pull/145

[jrouwe]

PathConstraintPathJS implements PathConstraintPath; was a typo, I fixed it. I don't think you'd ever want an implements and a JSImplementation for the same class as the code that that generates is nonsensical, so for me it's fine if it doesn't handle this case / triggers an error / implements it the way you current implemented it.

Thanks for fixing this!

[isaac-mason]

PathConstraintPathJS implements PathConstraintPath; was a typo, I fixed it. I don't think you'd ever want an implements and a JSImplementation for the same class as the code that that generates is nonsensical, so for me it's fine if it doesn't handle this case / triggers an error / implements it the way you current implemented it.

Gotcha, that makes sense. Perhaps it's worth at least emitting a warning when there's both implements and JSImplementation explaining the behaviour.

Thanks for fixing this!

No worries! 🙂