ronantakizawa/codereview-bench huggingface dataset:
Repository: playcanvas/engine
File: src/scene/batching/batch.js (JavaScript)

Code before:
        }
    }

    removeFromLayers(scene, layers) {
        for (let i = 0; i < layers.length; i++) {
            const layer = scene.layers.getLayerById(layers[i]);
            if (layer) {
                layer.removeMeshInstances([this.meshInstance]);
            }
        }
    }

    // Updates bounding box for a batch
    updateBoundingBox() {
        this._aabb.copy(this.origMeshInstances[0].aabb);
        for (let i = 1; i < this.origMeshInstances.length; i++) {
            this._aabb.add(this.origMeshInstances[i].aabb);
        }
        this.meshInstance.aabb = this._aabb;
        this.meshInstance._aabbVer = 0;
    }

    /**
     * @deprecated
     * @private
     * @returns {undefined}
     */
    get model() {
        Debug.removed('pc.Batch#model was removed. Use pc.Batch#meshInstance to access batched mesh instead.');
        return undefined;
    }
}

export { Batch };


Review comment (suggestion):
Getters/setters use `@type`, not `@returns`. See other examples.

Code after:
        }
    }

    removeFromLayers(scene, layers) {
        for (let i = 0; i < layers.length; i++) {
            const layer = scene.layers.getLayerById(layers[i]);
            if (layer) {
                layer.removeMeshInstances([this.meshInstance]);
            }
        }
    }

    // Updates bounding box for a batch
    updateBoundingBox() {
        this._aabb.copy(this.origMeshInstances[0].aabb);
        for (let i = 1; i < this.origMeshInstances.length; i++) {
            this._aabb.add(this.origMeshInstances[i].aabb);
        }
        this.meshInstance.aabb = this._aabb;
        this.meshInstance._aabbVer = 0;
    }

    /**
     * @deprecated
     * @ignore
     * @type {undefined}
     */
    get model() {
        Debug.removed('pc.Batch#model was removed. Use pc.Batch#meshInstance to access batched mesh instead.');
        return undefined;
    }
}

export { Batch };
Repository: playcanvas/engine
File: src/scene/batching/batch.js (JavaScript)

Code before:
            }
        }
    }

    removeFromLayers(scene, layers) {
        for (let i = 0; i < layers.length; i++) {
            const layer = scene.layers.getLayerById(layers[i]);
            if (layer) {
                layer.removeMeshInstances([this.meshInstance]);
            }
        }
    }

    // Updates bounding box for a batch
    updateBoundingBox() {
        this._aabb.copy(this.origMeshInstances[0].aabb);
        for (let i = 1; i < this.origMeshInstances.length; i++) {
            this._aabb.add(this.origMeshInstances[i].aabb);
        }
        this.meshInstance.aabb = this._aabb;
        this.meshInstance._aabbVer = 0;
    }

    /**
     * @deprecated
     * @private
     * @returns {undefined}
     */
    get model() {
        Debug.removed('pc.Batch#model was removed. Use pc.Batch#meshInstance to access batched mesh instead.');
        return undefined;
    }
}

export { Batch };


Review comment (suggestion):
This getter isn't private. It public, but presumably ignored from the documentation? So I suspect this should be `@ignore` instead.

Code after:
            }
        }
    }

    removeFromLayers(scene, layers) {
        for (let i = 0; i < layers.length; i++) {
            const layer = scene.layers.getLayerById(layers[i]);
            if (layer) {
                layer.removeMeshInstances([this.meshInstance]);
            }
        }
    }

    // Updates bounding box for a batch
    updateBoundingBox() {
        this._aabb.copy(this.origMeshInstances[0].aabb);
        for (let i = 1; i < this.origMeshInstances.length; i++) {
            this._aabb.add(this.origMeshInstances[i].aabb);
        }
        this.meshInstance.aabb = this._aabb;
        this.meshInstance._aabbVer = 0;
    }

    /**
     * @deprecated
     * @ignore
     * @type {undefined}
     */
    get model() {
        Debug.removed('pc.Batch#model was removed. Use pc.Batch#meshInstance to access batched mesh instead.');
        return undefined;
    }
}

export { Batch };
Repository: playcanvas/engine
File: src/framework/components/render/component.js (JavaScript)

Code before:
    /**
     * @type {AssetReference}
     * @private
     */
    _assetReference;

    /**
     * @type {AssetReference[]}
     * @private
     */
    _materialReferences = [];

    /**
     * Material used to render meshes other than asset type. It gets priority when set to
     * something else than defaultMaterial, otherwise materialASsets[0] is used.
     *
     * @type {import('../../../scene/materials/material.js').Material}
     * @private
     */
    _material;

    /**
     * A reference to the entity to be used as the root bone for any skinned meshes that
     * are rendered by this component.
     *
     * @type {import('../../entity.js').Entity|null}
     */
    _rootBone = null;

    /**
     * @type {import('../../../core/event-handle.js').EventHandle|null}
     * @private
     */
    _evtLayersChanged = null;

    /**
     * @type {import('../../../core/event-handle.js').EventHandle|null}
     * @private
     */
    _evtLayerAdded = null;

    /**
     * @type {import('../../../core/event-handle.js').EventHandle|null}
     * @private
     */
    _evtLayerRemoved = null;

    /**
     * @type {import('../../../core/event-handle.js').EventHandle|null}
     * @private
     */

Review comment (suggestion):
You've made the private class field `_rootBone` public....but instead, you need to reinstate the `@private` tag here and instead, document the setter for `rootBone` below.

Code after:
     * @ignore
     */
    _area = null;

    /**
     * @type {AssetReference}
     * @private
     */
    _assetReference;

    /**
     * @type {AssetReference[]}
     * @private
     */
    _materialReferences = [];

    /**
     * Material used to render meshes other than asset type. It gets priority when set to
     * something else than defaultMaterial, otherwise materialASsets[0] is used.
     *
     * @type {import('../../../scene/materials/material.js').Material}
     * @private
     */
    _material;

    /**
     * A reference to the entity to be used as the root bone for any skinned meshes that
     * are rendered by this component.
     *
     * @type {Entity|null}
     * @private
     */
    _rootBone = null;

    /**
     * @type {import('../../../core/event-handle.js').EventHandle|null}
     * @private
     */
    _evtLayersChanged = null;

    /**
     * @type {import('../../../core/event-handle.js').EventHandle|null}
     * @private
     */
    _evtLayerAdded = null;

    /**
     * @type {import('../../../core/event-handle.js').EventHandle|null}
     * @private
     */
    _evtLayerRemoved = null;
Repository: playcanvas/engine
File: src/framework/components/render/component.js (JavaScript)

Code before:
     */
    assignAsset(asset) {
        const id = asset instanceof Asset ? asset.id : asset;
        this._assetReference.id = id;
    }

    /**
     * Sets the root bone entity (or entity guid) for the render component.
     *
     * @type {Entity|string|null}
     */
    set rootBone(value) {
        if (this._rootBone !== value) {
            const isString = typeof value === 'string';
            if (this._rootBone && isString && this._rootBone.getGuid() === value) {
                return;
            }

            if (this._rootBone) {
                this._clearSkinInstances();
            }

            if (value instanceof GraphNode) {
                this._rootBone = value;
            } else if (isString) {
                this._rootBone = this.system.app.getEntityFromIndex(value) || null;
            } else {
                this._rootBone = null;
            }

            if (this._rootBone) {
                this._cloneSkinInstances();
            }
        }
    }

    /**
     * Gets the root bone entity for the render component.
     *
     * @type {Entity|null}
     */
    get rootBone() {
        return this._rootBone;
    }

    /** @private */
    destroyMeshInstances() {
        const meshInstances = this._meshInstances;
        if (meshInstances) {
            this.removeFromLayers();


Review comment (bug):
I would be tempted to add Debug.warn if this._rootBone is null after this line - to inform the user that the guid didn't resolve.

Code after:
     */
    assignAsset(asset) {
        const id = asset instanceof Asset ? asset.id : asset;
        this._assetReference.id = id;
    }

    /**
     * Sets the root bone entity (or entity guid) for the render component.
     *
     * @type {Entity|string|null}
     */
    set rootBone(value) {
        if (this._rootBone !== value) {
            const isString = typeof value === 'string';
            if (this._rootBone && isString && this._rootBone.getGuid() === value) {
                return;
            }

            if (this._rootBone) {
                this._clearSkinInstances();
            }

            if (value instanceof GraphNode) {
                this._rootBone = value;
            } else if (isString) {
                this._rootBone = this.system.app.getEntityFromIndex(value) || null;
                if (!this._rootBone) {
                    Debug.warn('Failed to find rootBone Entity by GUID');
                }
            } else {
                this._rootBone = null;
            }

            if (this._rootBone) {
                this._cloneSkinInstances();
            }
        }
    }

    /**
     * Gets the root bone entity for the render component.
     *
     * @type {Entity|null}
     */
    get rootBone() {
        return this._rootBone;
    }

    /** @private */
    destroyMeshInstances() {
        const meshInstances = this._meshInstances;
Repository: playcanvas/engine
File: src/framework/components/button/component.js (JavaScript)

Code before:
            this._updateTintTween();
        }
    }

    onEnable() {
        // Reset input state
        this._isHovering = false;
        this._hoveringCounter = 0;
        this._isPressed = false;

        this._imageReference.onParentComponentEnable();
        this._toggleHitElementListeners('on');
        this._forceReapplyVisualState();
    }

    onDisable() {
        this._toggleHitElementListeners('off');
        this._resetToDefaultVisualState(this.transitionMode);
    }

    onRemove() {
        this._toggleLifecycleListeners('off', this.system);
        this.onDisable();
    }
}

function toColor3(color4) {
    return new Color(color4.r, color4.g, color4.b);
}

export { ButtonComponent };


Review comment (suggestion):
should this be just a default value for the parameter?
```
_applySprite(spriteAsset, spriteFrame = 0) {
```

Code after:

            case BUTTON_TRANSITION_MODE_SPRITE_CHANGE:
                this._applySprite(this._defaultSpriteAsset, this._defaultSpriteFrame);
                break;
        }
    }

    _determineVisualState() {
        if (!this.active) {
            return VisualState.INACTIVE;
        } else if (this._isPressed) {
            return VisualState.PRESSED;
        } else if (this._isHovering) {
            return VisualState.HOVER;
        }

        return VisualState.DEFAULT;
    }

    _applySprite(spriteAsset, spriteFrame) {
        const element = this._imageEntity?.element;
        if (!element) {
            return;
        }

        spriteFrame = spriteFrame || 0;

        this._isApplyingSprite = true;

        if (element.spriteAsset !== spriteAsset) {
            element.spriteAsset = spriteAsset;
        }

        if (element.spriteFrame !== spriteFrame) {
            element.spriteFrame = spriteFrame;
        }

        this._isApplyingSprite = false;
    }

    _applyTint(tintColor) {
        this._cancelTween();

        if (this.fadeDuration === 0) {
            this._applyTintImmediately(tintColor);
        } else {
            this._applyTintWithTween(tintColor);
        }
    }

    _applyTintImmediately(tintColor) {
Repository: playcanvas/engine
File: src/scene/gsplat/gsplat.js (JavaScript)

Code before:
};

/** @ignore */
class GSplat {
    device;

    numSplats;

    numSplatsVisible;

    /** @type {Float32Array} */
    centers;

    /** @type {BoundingBox} */
    aabb;

    /** @type {Texture} */
    colorTexture;

    /** @type {Texture} */
    transformATexture;

    /** @type {Texture} */
    transformBTexture;

    /** @type {number} */
    shBands;

    /** @type {Texture | undefined} */
    sh1to3Texture;

    /** @type {Texture | undefined} */
    sh4to7Texture;

    /** @type {Texture | undefined} */
    sh8Texture;

    /** @type {Texture | undefined} */
    sh8to11Texture;

    /** @type {Texture | undefined} */
    sh12to15Texture;

    /**
     * @param {GraphicsDevice} device - The graphics device.
     * @param {GSplatData} gsplatData - The splat data.
     */
    constructor(device, gsplatData) {
        const numSplats = gsplatData.numSplats;

        this.device = device;

Review comment (suggestion):
```suggestion
    /** @type {0 | 1 | 2 | 3} */
```

Code after:
};

/** @ignore */
class GSplat {
    device;

    numSplats;

    numSplatsVisible;

    /** @type {Float32Array} */
    centers;

    /** @type {BoundingBox} */
    aabb;

    /** @type {Texture} */
    colorTexture;

    /** @type {Texture} */
    transformATexture;

    /** @type {Texture} */
    transformBTexture;

    /** @type {0 | 1 | 2 | 3} */
    shBands;

    /** @type {Texture | undefined} */
    sh1to3Texture;

    /** @type {Texture | undefined} */
    sh4to7Texture;

    /** @type {Texture | undefined} */
    sh8Texture;

    /** @type {Texture | undefined} */
    sh8to11Texture;

    /** @type {Texture | undefined} */
    sh12to15Texture;

    /**
     * @param {GraphicsDevice} device - The graphics device.
     * @param {GSplatData} gsplatData - The splat data.
     */
    constructor(device, gsplatData) {
        const numSplats = gsplatData.numSplats;

        this.device = device;
Repository: playcanvas/engine
File: src/core/math/vec2.js (JavaScript)

Code before:
     * Each element is assigned a value from rhs parameter if it is larger.
     *
     * @param {Vec2} rhs - The 2-dimensional vector used as the source of elements to compare to.
     * @returns {Vec2} Self for chaining.
     */
    max(rhs) {
        if (rhs.x > this.x) this.x = rhs.x;
        if (rhs.y > this.y) this.y = rhs.y;
        return this;
    }

    /**
     * Sets the specified 2-dimensional vector to the supplied numerical values.
     *
     * @param {number | number[]} x - The value to set on the first component of the vector.
     * @param {number} y - The value to set on the second component of the vector.
     * @returns {Vec2} Self for chaining.
     * @example
     * const v = new pc.Vec2();
     * v.set(5, 10);
     *
     * // Outputs 5, 10
     * console.log("The result of the vector set is: " + v.toString());
     */
    set(x, y) {
        if (x.length === 2) {
            this.x = x[0];
            this.y = x[1];
        } else {
            this.x = x;
            this.y = y;
        }

        return this;
    }

    /**
     * Subtracts a 2-dimensional vector from another in place.
     *
     * @param {Vec2} rhs - The vector to subtract from the specified vector.
     * @returns {Vec2} Self for chaining.
     * @example
     * const a = new pc.Vec2(10, 10);
     * const b = new pc.Vec2(20, 20);
     *
     * a.sub(b);
     *
     * // Outputs [-10, -10]
     * console.log("The result of the subtraction is: " + a.toString());
     */
    sub(rhs) {

Review comment (suggestion):
```suggestion
        if (x?.length === 2) {
```

Code after:
     * Each element is assigned a value from rhs parameter if it is larger.
     *
     * @param {Vec2} rhs - The 2-dimensional vector used as the source of elements to compare to.
     * @returns {Vec2} Self for chaining.
     */
    max(rhs) {
        if (rhs.x > this.x) this.x = rhs.x;
        if (rhs.y > this.y) this.y = rhs.y;
        return this;
    }

    /**
     * Sets the specified 2-dimensional vector to the supplied numerical values.
     *
     * @param {number} x - The value to set on the first component of the vector.
     * @param {number} y - The value to set on the second component of the vector.
     * @returns {Vec2} Self for chaining.
     * @example
     * const v = new pc.Vec2();
     * v.set(5, 10);
     *
     * // Outputs 5, 10
     * console.log("The result of the vector set is: " + v.toString());
     */
    set(x, y) {
        this.x = x;
        this.y = y;

        return this;
    }

    /**
     * Subtracts a 2-dimensional vector from another in place.
     *
     * @param {Vec2} rhs - The vector to subtract from the specified vector.
     * @returns {Vec2} Self for chaining.
     * @example
     * const a = new pc.Vec2(10, 10);
     * const b = new pc.Vec2(20, 20);
     *
     * a.sub(b);
     *
     * // Outputs [-10, -10]
     * console.log("The result of the subtraction is: " + a.toString());
     */
    sub(rhs) {
        this.x -= rhs.x;
        this.y -= rhs.y;

        return this;
    }
Repository: playcanvas/engine
File: scripts/esm/camera-frame.mjs (JavaScript)

Code before:
     * @type {ToneMapping}
     */
    toneMapping = ToneMapping.LINEAR;

    /**
     * @range [0, 1]
     * @precision 3
     * @step 0.001
     */
    sharpness = 0.0;

    /**
     * @attribute
     * @type {DebugType}
     */
    debug = DebugType.NONE;
}

/** @interface */
class Ssao {
    /**
     * @attribute
     * @type {SsaoType}
     */
    type = SsaoType.NONE;

    blurEnabled = true;

    /**
     * @range [0, 1]
     * @precision 3
     * @step 0.001
     */
    intensity = 0.5;

    /**
     * @range [0, 100]
     * @precision 3
     * @step 0.001
     */
    radius = 30;

    /**
     * @range [1, 64]
     * @precision 0
     * @step 1
     */
    samples = 12;

    /**
     * @range [0.1, 10]

Review comment (suggestion):
@mvaligursky This is another reason why I wanted an `enabled` property on SSAO. It's gonna look quite incongruous against the other effects in the Editor UI (where everything else can be toggled in the effect header bar).

Code after:
    toneMapping = ToneMapping.LINEAR;

    /**
     * @range [0, 1]
     * @precision 3
     * @step 0.001
     */
    sharpness = 0.0;

    /**
     * @attribute
     * @type {DebugType}
     */
    debug = DebugType.NONE;
}

/** @interface */
class Ssao {
    /**
     * @attribute
     * @type {SsaoType}
     */
    type = SsaoType.NONE;

    /**
     * @visibleif {type !== 'none'}
     */
    blurEnabled = true;

    /**
     * @range [0, 1]
     * @visibleif {type !== 'none'}
     * @precision 3
     * @step 0.001
     */
    intensity = 0.5;

    /**
     * @range [0, 100]
     * @visibleif {type !== 'none'}
     * @precision 3
     * @step 0.001
     */
    radius = 30;

    /**
     * @range [1, 64]
     * @visibleif {type !== 'none'}
     * @precision 0
     * @step 1
     */
Repository: playcanvas/engine
File: src/scene/shader-lib/chunks-wgsl/internal/morph/vert/morph.js (JavaScript)

Code before:
// vertex shader internally used to apply morph targets in textures into a final texture containing
// blended morph targets
export default /* wgsl */`
    attribute vertex_position: vec2f;
    varying uv0: vec2f;

    @vertex
    fn vertexMain(input: VertexInput) -> VertexOutput {
      var output: VertexOutput;
      output.position = vec4f(input.vertex_position, 0.5, 1.0);
      output.uv0 = input.vertex_position * 0.5 + vec2f(0.5, 0.5);
      return output;
    }
`;


Review comment (suggestion):
```suggestion
        var output: VertexOutput;
        output.position = vec4f(input.vertex_position, 0.5, 1.0);
        output.uv0 = input.vertex_position * 0.5 + vec2f(0.5, 0.5);
        return output;
```

Code after:
// vertex shader internally used to apply morph targets in textures into a final texture containing
// blended morph targets
export default /* wgsl */`
    attribute vertex_position: vec2f;
    varying uv0: vec2f;

    @vertex
    fn vertexMain(input: VertexInput) -> VertexOutput {
        var output: VertexOutput;
        output.position = vec4f(input.vertex_position, 0.5, 1.0);
        output.uv0 = input.vertex_position * 0.5 + vec2f(0.5, 0.5);
        return output;
    }
`;
Repository: playcanvas/engine
File: scripts/esm/camera-frame.mjs (JavaScript)

Code before:
     */
    saturation = 1;

    /**
     * @attribute
     * @visibleif {enabled}
     */
    tint = new Color(1, 1, 1, 1);
}

/** @interface */
class ColorLUT {
    /**
     * @attribute
     * @type {Asset}
     * @resource texture
     */
    texture = null;

    /**
     * @range [0, 1]
     * @precision 3
     * @step 0.001
     */
    intensity = 1;
}

/** @interface */
class Vignette {
    enabled = false;

    /**
     * @visibleif {enabled}
     * @range [0, 1]
     * @precision 3
     * @step 0.001
     */
    intensity = 0.5;

    /**
     * @visibleif {enabled}
     * @range [0, 3]
     * @precision 3
     * @step 0.001
     */
    inner = 0.5;

    /**
     * @visibleif {enabled}
     * @range [0, 3]
     * @precision 3

Review comment (suggestion):
You can use `@visibleif {texture}` here instead @mvaligursky . The parser isn't actually working with null at the moment

Code after:
     * @visibleif {enabled}
     * @range [0, 2]
     * @precision 3
     * @step 0.001
     */
    saturation = 1;

    /**
     * @attribute
     * @visibleif {enabled}
     */
    tint = new Color(1, 1, 1, 1);
}

/** @interface */
class ColorLUT {
    /**
     * @attribute
     * @type {Asset}
     * @resource texture
     */
    texture = null;

    /**
     * @visibleif {texture}
     * @range [0, 1]
     * @precision 3
     * @step 0.001
     */
    intensity = 1;
}

/** @interface */
class Vignette {
    enabled = false;

    /**
     * @visibleif {enabled}
     * @range [0, 1]
     * @precision 3
     * @step 0.001
     */
    intensity = 0.5;

    /**
     * @visibleif {enabled}
     * @range [0, 3]
     * @precision 3
     * @step 0.001
     */
    inner = 0.5;