Mugen87/yuka

jsdoc contradiction to code in GameEntity

discordier opened this issue · 16 comments

discordier commented

I just created typescript definitions for all types in yuka (I wonder if I should submit them as PR here) and found that we have a contradiction in return types in GameEntity.js

yuka/src/core/GameEntity.js

Lines 202 to 216 in 6314fd6

/**
* Executed when this game entity is updated for the first time by its {@link EntityManager}.
*
* @return {GameEntity} A reference to this game entity.
*/
start() {}
/**
* Updates the internal state of this game entity. Normally called by {@link EntityManager#update}
* in each simulation step.
*
* @param {Number} delta - The time delta.
* @return {GameEntity} A reference to this game entity.
*/
update( /* delta */ ) {}

See, we define that we are returning this, while we are in fact a void method.

I wonder what the real intention is here, as I personally do not see any benefit in chaining either method (To be honest, most methods in GameEntity won't be chained in real life applications).

Shall we now add the missing return this or update the jsdoc to reflect @return void?

Mugen87 commented

I think I would prefer to add the missing return this statements. Do you mind creating a PR?

I just created typescript definitions for all types in yuka (I wonder if I should submit them as PR here)

Many thanks for this! According to my experience with three.js, I suggest to add these types to https://github.com/DefinitelyTyped/DefinitelyTyped.

discordier commented

I think I would prefer to add the missing return this statements. Do you mind creating a PR?

See #44

I just created typescript definitions for all types in yuka (I wonder if I should submit them as PR here)

Many thanks for this! According to my experience with three.js, I suggest to add these types to https://github.com/DefinitelyTyped/DefinitelyTyped.

I had a quick glance at the README over there and might give it a shot.
I think to do so, we would be in need of releases/tags, in this repository won't we? Otherwise it is a pretty hard choice for me to decide when update of types is needed.
I mean, we are currently on 0.7.3 but which commit is it? three.js has proper tags.

Another possibility would be to convert the code base entirely to typescript and recompile it to es6 but I guess you would not want that.

While doing the conversion, there were some discrepancies I noticed (sadly I forgot to write them down as I was in a hurry), would you prefer to receive separate tickets on each of these?

Mugen87 commented

I guess I have to start using tags now 😅 .

Another possibility would be to convert the code base entirely to typescript and recompile it to es6 but I guess you would not want that.

No, sorry.

While doing the conversion, there were some discrepancies I noticed (sadly I forgot to write them down as I was in a hurry), would you prefer to receive separate tickets on each of these?

I think you can share them right here in this issue.

Mugen87 commented

There is now a tag for the new 0.7.4.

https://github.com/Mugen87/yuka/releases/tag/v0.7.4

  • 👍1
discordier commented

I think you can share them right here in this issue.

Ok, here they come:

  1. class Triangle is undefined (reference here).
    I mitigated by defining an interface as such:
export interface Triangle {
    /**
     * The first vertex position.
     */
    a: Vector3;
    /**
     * The second vertex position.
     */
    b: Vector3;
    /**
     * The third vertex position.
     */
    c: Vector3;
}

Where in yuka should this be placed?

  1. OBB states that it wants a rotation: Quaternion for constructor and set() while in fact is using a Matrix3. This should be changed.

    yuka/src/math/OBB.js

    Lines 50 to 52 in 031df15

    * @param {Quaternion} rotation - The rotation of this OBB.
    */
    constructor( center = new Vector3(), halfSizes = new Vector3(), rotation = new Matrix3() ) {

    yuka/src/math/OBB.js

    Lines 79 to 82 in 031df15

    * @param {Quaternion} rotation - The rotation of this OBB.
    * @return {OBB} A reference to this OBB.
    */
    set( center, halfSizes, rotation ) {

  2. OBB param comment is invalid, should be obb instead of aabb.

    yuka/src/math/OBB.js

    Lines 602 to 603 in 031df15

    * @param {OBB} aabb - The OBB to test.
    * @return {Boolean} The result of the equality test.

  3. MathUtils.clamp() min and max are named wrong:

    yuka/src/math/MathUtils.js

    Lines 88 to 93 in 031df15

    * @param {Number} value - The value to clamp.
    * @param {min} value - The min value.
    * @param {max} value - The max value.
    * @return {Number} The clamped value.
    */
    static clamp( value, min, max ) {

    same here

    yuka/src/math/MathUtils.js

    Lines 124 to 128 in 031df15

    * @param {min} value - The min value.
    * @param {max} value - The max value.
    * @return {Number} The random float value.
    */
    static randFloat( min, max ) {

    and here

    yuka/src/math/MathUtils.js

    Lines 137 to 141 in 031df15

    * @param {min} value - The min value.
    * @param {max} value - The max value.
    * @return {Number} The random integer value.
    */
    static randInt( min, max ) {

  4. There are several nullable props in HalfEdge which denote per jsdoc that they are not.
    See: https://github.com/Mugen87/yuka/blob/031df15b4990db59a294d3ecfb6543474b027aed/src/math/HalfEdge.js
    The following are all nullable: next, prev, twin, polygon.

  5. All graph properties in all src/graph/search/*.js are in fact nullable (as initialized with null) but state that they are not.

  6. All properties in src/fsm/StateMachine.js are in fact nullable (as initialized with null) but state that they are not.

  7. MeshGeometry.indices is nullable

These are the ones that I could reconstruct.
If you agree on above points and clearify how you want it, I could create a follow up PR which changes these.

  • 👀1
Mugen87 commented

I have quickly fixed point 2, 3 and 4 (8fdd2a9). However, I am not going to create a separate class for the inline type Triangle.

The JSDoc for nullable properties would look like so, right?

* @type {HalfEdge | null}
discordier commented

I have quickly fixed point 2, 3 and 4 (8fdd2a9). However, I am not going to create a separate class for the inline type Triangle.

You don't have to, it should be sufficient to add the following anywhere in the same file.
This transports the information of the expected shape without creating "real" code.

/**
 * A triangle shape used in {@link Ray#intersectTriangle}.
 *
 * @typedef {Object} Triangle
 * @property {Vector3} a The first vertex position.
 * @property {Vector3} b The second vertex position.
 * @property {Vector3} c The third vertex position.
 */

The JSDoc for nullable properties would look like so, right?

* @type {HalfEdge | null}

Yes, this is correct.

However, I think some of the properties should become mandatory.
Sadly the serialization feature currently prevents us from doing so for many properties.
i.e. every Goal needs an owner, doesn't it?
However, the property is nullable, because of the constructor must be callable without arguments and therefore the object is theorethically without parent for some cycles (until foo.resolveReferences() has resolved the entity).
To make things worse, the value owner can in fact be a string (for the time in between .fromJSON() and .resolveReferences()).
IMO the serialization/deserialization should be implemented with dedicated classes instead of baked into the concrete classes.
This is already half the case for every registerType handling.
Would you mind me refactoring it out into separate classes?
Side effect of more cleaner code would be that the classes will also shrink when built into applications due to tree shaking, as the serialization would only be included when explicitly used vs. always as is the current case.

Mugen87 commented

IMO the serialization/deserialization should be implemented with dedicated classes instead of baked into the concrete classes.

In three.js we have followed a more centralized approach for serializing/deserializing however this leaks component related logic to external entities (e.g. ObjectLoader). Hence, it was considered to introduce fromJSON() and toJSON() methods in all related classes similar to Yuka (see mrdoob/three.js#11266). I guess I want to keep this approach since it's in my opinion the more clear way. Even it means slightly larger bundles.

Mugen87 commented

You don't have to, it should be sufficient to add the following anywhere in the same file.
This transports the information of the expected shape without creating "real" code.

Nice! Did not know this was possible. 3d0aefc

  • 👍1
discordier commented

IMO the serialization/deserialization should be implemented with dedicated classes instead of baked into the concrete classes.

In three.js we have followed a more centralized approach for serializing/deserializing however this leaks component related logic to external entities (e.g. ObejctLoader). Hence, it was considered to introduce fromJSON() and toJSON() methods in all related classes similar to Yuka (see mrdoob/three.js#11266).

The issue is still open and I fail to see a definitive answer there, as Don McCurdy (intentionally not linked here, did not want to cause a notification) pointed out, a factory is needed to create the instances properly.
However, it does not end there, as the dependencies are not ordered and in fact, can be circular in yuka (hence the *.resolveReferences() methods.

I guess I want to keep this approach since it's in my opinion the more clear way. Even it means slightly larger bundles.

Larger bundles were not my main concern but rather the non strict types for properties as mentioned above.

Another solution would be, to add static fromJSON() methods to the prototypes that resolve lazy.
That way, only the constructor must be registered (already the case on the various classes) and a static method must be defined on the prototype. This would make the fromJSON() method a little more complex but allows strict types everywhere.
Furthermore, all shipped classes (GameEntity, MovingEntity etc.) would also have be registered and would not be special cases anymore.
However, I see that this is a pretty large change and will put it to rest for the moment. Maybe I will throw together an alternative example in the future.

Mugen87 commented

I've investigated the nullable issue a bit more. Instead of adding | null, it should be sufficient to add the question mark as a prefix. According to the docs:

This indicates that the type is either the specified type, or null.

Do you see any issue with that approach? If not, I would like to adapt it since the JSDoc generator will create a nice nullable annotation in the HTML (similar to readonly).

discordier commented

I don't see any problem with that as it appears that in jsdoc ?Foo appears to be syntactic sugar for null | Foo (or Foo | null respectively) so it both means the same.
In the end, we only need to ensure/annotate that we do not allow undefined here.

Mugen87 commented

Okay, all nullable properties should now be documented correctly 👍 . 2e2d1bd

  • 👍1
discordier commented

For the record, the types have just been published as: @types/yuka.
If anyone want's to improve, feel welcome to become co-maintainer.

  • 🎉1
trusktr commented

the types have just been published as: @types/yuka.

@discordier How does it work? Do the types get compiled from the source JSDocs? Or maintaining both by hand?