ronantakizawa/codereview-bench huggingface dataset:
Repository: pixijs/pixijs
File: tests/scene/checkChildrenDidChange.tests.ts (TypeScript)

Code before:
import { Container } from '../../src/scene/container/Container';
import { checkChildrenDidChange } from '../../src/scene/container/utils/checkChildrenDidChange';
import { getChangeId } from '../../src/scene/container/utils/getChangeId';

describe('checkChildrenDidChange', () =>
{
    it('should register a change past 4096', async () =>
    {
        const container = new Container();
        const changeId = getChangeId(container);

        expect(changeId).toEqual(0);

        container._didContainerChangeTick = 4096;

        const newChangeId = getChangeId(container);

        expect(newChangeId).toEqual(1);
    });

    it('should correctly manage ids', async () =>
    {
        const container = new Container();

        const child = new Container();

        container.addChild(child);

        const previousData = {
            data: [] as number[],
            index: 0,
            didChange: false,
        };

        checkChildrenDidChange(container, previousData);

        let childChange = getChangeId(child);

        expect(previousData).toEqual({

Review comment (bug):
The test will fail if you change it to 4095. The weird math (`(x % 0xFFF) & 0xFFF`) just makes it wrap 1 earlier.

> (4095 % 0xFFF) & 0xFFF === 0

`(x % 0xFFF) & 0xFFF` isn't any better than `x & 0xFFF`. In fact it's a tiny bit worse.

Code after:
import { Container } from '../../src/scene/container/Container';
import { checkChildrenDidChange } from '../../src/scene/container/utils/checkChildrenDidChange';

describe('checkChildrenDidChange', () =>
{
    it('should correctly manage ids', async () =>
    {
        const container = new Container();

        const child = new Container();

        container.addChild(child);

        const previousData = {
            data: [] as number[],
            index: 0,
            didChange: false,
        };

        checkChildrenDidChange(container, previousData);

        let childChange = ((child._didViewChangeTick & 0xffff) << 16) | (child._didContainerChangeTick & 0xffff);

        expect(previousData).toEqual({
            data: [child.uid, childChange],
            index: 2,
            didChange: true,
        });

        previousData.index = 0;
        previousData.didChange = false;

        checkChildrenDidChange(container, previousData);

        expect(previousData).toEqual({
            data: [child.uid, childChange],
            index: 2,
            didChange: false,
        });
Repository: pixijs/pixijs
File: src/rendering/batcher/shared/DefaultBatcher.ts (TypeScript)

Code before:
import { ExtensionType } from '../../../extensions/Extensions';
import { Batcher } from './Batcher';
import { BatchGeometry } from './BatchGeometry';
import { DefaultShader } from './DefaultShader';

import type { Matrix } from '../../../maths/matrix/Matrix';
import type { Shader } from '../../renderers/shared/shader/Shader';
import type { BatchableMeshElement, BatchableQuadElement } from './Batcher';

let defaultShader: Shader = null;

interface DefaultBatchElements
{
    /** The color of the element that will be multiplied with the texture color. */
    color: number;

    /** Whether the element should be rounded to the nearest pixel. */
    roundPixels: 0 | 1;

    /** The transform of the element. */
    transform: Matrix;
}

export interface DefaultBatchableQuadElement extends BatchableQuadElement, DefaultBatchElements {}
export interface DefaultBatchableMeshElement extends BatchableMeshElement, DefaultBatchElements {}

/** The default batcher is used to batch quads and meshes. */
export class DefaultBatcher extends Batcher
{
    /** @ignore */
    public static extension = {
        type: [
            ExtensionType.Batcher,
        ],
        name: 'default',
    } as const;


Review comment (performance):
```suggestion
export interface DefaultBatchElements
```

Code after:
import { ExtensionType } from '../../../extensions/Extensions';
import { Batcher } from './Batcher';
import { BatchGeometry } from './BatchGeometry';
import { DefaultShader } from './DefaultShader';

import type { Matrix } from '../../../maths/matrix/Matrix';
import type { Shader } from '../../renderers/shared/shader/Shader';
import type { BatchableMeshElement, BatchableQuadElement } from './Batcher';

let defaultShader: Shader = null;

export interface DefaultBatchElements
{
    /** The color of the element that will be multiplied with the texture color. */
    color: number;

    /** Whether the element should be rounded to the nearest pixel. */
    roundPixels: 0 | 1;

    /** The transform of the element. */
    transform: Matrix;
}

export interface DefaultBatchableQuadElement extends BatchableQuadElement, DefaultBatchElements {}
export interface DefaultBatchableMeshElement extends BatchableMeshElement, DefaultBatchElements {}

/** The default batcher is used to batch quads and meshes. */
export class DefaultBatcher extends Batcher
{
    /** @ignore */
    public static extension = {
        type: [
            ExtensionType.Batcher,
        ],
        name: 'default',
    } as const;
Repository: pixijs/pixijs
File: src/scene/container/container-mixins/effectsMixin.ts (TypeScript)

Code before:
    addEffect(effect: Effect)
    {
        const index = this.effects.indexOf(effect);

        if (index !== -1) return; // already exists!

        this.effects.push(effect);

        this.effects.sort((a, b) => a.priority - b.priority);

        this._markStructureDidChange();

        // if (this.renderGroup)
        // {
        //     this.renderGroup.structureDidChange = true;
        // }

        this._updateIsSimple();
    },
    /**
     * @todo Needs docs.
     * @param effect - The effect to remove.
     * @memberof scene.Container#
     * @ignore
     */
    removeEffect(effect: Effect)
    {
        const index = this.effects.indexOf(effect);

        if (index === -1) return; // already exists!

        this.effects.splice(index, 1);

        this._markStructureDidChange();

        this._updateIsSimple();
    },

    set mask(value: Mask)
    {
        const effect = this._maskEffect;

        if (effect?.mask === value) return;

        if (effect)
        {
            this.removeEffect(effect);

            MaskEffectManager.returnMaskEffect(effect);

            this._maskEffect = null;

Review comment (bug):
I assume that when effect is removed it should has same logic as `addEffect` for getting rendering group.
Please correct me if I'm wrong.

Code after:
    addEffect(effect: Effect)
    {
        const index = this.effects.indexOf(effect);

        if (index !== -1) return; // already exists!

        this.effects.push(effect);

        this.effects.sort((a, b) => a.priority - b.priority);

        this._markStructureAsChanged();

        // if (this.renderGroup)
        // {
        //     this.renderGroup.structureDidChange = true;
        // }

        this._updateIsSimple();
    },
    /**
     * @todo Needs docs.
     * @param effect - The effect to remove.
     * @memberof scene.Container#
     * @ignore
     */
    removeEffect(effect: Effect)
    {
        const index = this.effects.indexOf(effect);

        if (index === -1) return; // already exists!

        this.effects.splice(index, 1);

        this._markStructureAsChanged();

        this._updateIsSimple();
    },

    set mask(value: Mask)
    {
        const effect = this._maskEffect;

        if (effect?.mask === value) return;

        if (effect)
        {
            this.removeEffect(effect);

            MaskEffectManager.returnMaskEffect(effect);

            this._maskEffect = null;
Repository: pixijs/pixijs
File: tests/visual/scenes/particle/particle.scene.ts (TypeScript)

Code before:
        const textures = await Assets.load([
            `bunny.png`,
            'profile-abel@2x.jpg',
        ]);

        const texture = textures[`bunny.png`];
        const texture2 = textures[`profile-abel@2x.jpg`];

        const particleContainer = new ParticleContainer();

        for (let i = 0; i < 100; i++)
        {
            const particle = new Particle(texture);

            particle.x = (i % 10) * 10;
            particle.y = ((i / 10) | 0) * 10;

            particle.rotation = i * 0.01;

            particleContainer.addParticle(particle);
        }

        scene.addChild(particleContainer);

        const particleContainer2 = new ParticleContainer();

        const particle = new Particle(texture2);

        particle.anchorX = 0.5;
        particle.anchorY = 0.5;
        particle.x = 128 / 2;
        particle.y = 128 / 2;
        particle.scaleX = 0.6;
        particle.scaleY = 0.6;

        particleContainer2.addParticle(particle);

        scene.addChild(particleContainer2);
    },
};


Review comment (suggestion):
The tint value 'red' should be a hexadecimal color value. Use a value like 0xFF0000 instead.
```suggestion
                particle.tint = 0xFF0000;
```

Code after:
        const textures = await Assets.load([
            `bunny.png`,
            'profile-abel@2x.jpg',
        ]);

        const texture = textures[`bunny.png`];
        const texture2 = textures[`profile-abel@2x.jpg`];

        const particleContainer = new ParticleContainer();

        for (let i = 0; i < 100; i++)
        {
            const particle = new Particle(texture);

            particle.x = (i % 10) * 10;
            particle.y = ((i / 10) | 0) * 10;

            particle.rotation = i * 0.01;

            particleContainer.addParticle(particle);

            particle.alpha = i / 100;

            if (i % 2 === 0)
            {
                particle.tint = 'red';
            }
        }

        scene.addChild(particleContainer);

        const particleContainer2 = new ParticleContainer();

        const particle = new Particle(texture2);

        particle.anchorX = 0.5;
        particle.anchorY = 0.5;
        particle.x = 128 / 2;
        particle.y = 128 / 2;
        particle.scaleX = 0.6;
        particle.scaleY = 0.6;

        particleContainer2.addParticle(particle);

        scene.addChild(particleContainer2);
    },
};
Repository: pixijs/pixijs
File: src/scene/container/container-mixins/cacheAsTextureMixin.ts (TypeScript)

Code before:

    /**
     * Updates the cached texture. Will flag that this container's cached texture needs to be redrawn.
     * This will happen on the next render.
     * @memberof scene.Container#
     */
    updateCacheTexture(): void
    {
        this.renderGroup?.updateCacheTexture();
    },

    /**
     * Allows backwards compatibility with pixi.js below version v8. Use `cacheAsTexture` instead.
     * @deprecated
     */
    get cacheAsBitmap(): boolean
    {
        return this.isCachedAsTexture;
    },

    /**
     * @deprecated
     */
    set cacheAsBitmap(val: boolean)
    {
        warn('cacheAsBitmap is deprecated, use cacheAsTexture instead.');
        this.cacheAsTexture(val);
    },
} as Container;


Review comment (bug):
Can we use deprecation instead? and make sure to wrap in if/endif so that it's removed from release builds.

```suggestion
        // #if _DEBUG
        deprecation('cacheAsBitmap is deprecated, use cacheAsTexture instead.');
        // #endif
```

Code after:

    /**
     * Updates the cached texture. Will flag that this container's cached texture needs to be redrawn.
     * This will happen on the next render.
     * @memberof scene.Container#
     */
    updateCacheTexture(): void
    {
        this.renderGroup?.updateCacheTexture();
    },

    /**
     * Allows backwards compatibility with pixi.js below version v8. Use `cacheAsTexture` instead.
     * @deprecated
     */
    get cacheAsBitmap(): boolean
    {
        return this.isCachedAsTexture;
    },

    /**
     * @deprecated
     */
    set cacheAsBitmap(val: boolean)
    {
        // #if _DEBUG
        deprecation('v8.6.0', 'cacheAsBitmap is deprecated, use cacheAsTexture instead.');
        // #endif
        this.cacheAsTexture(val);
    },
} as Container;
Repository: pixijs/pixijs
File: src/scene/sprite/Sprite.ts (TypeScript)

Code before:
    }

    public override onViewUpdate()
    {
        this._boundsDirty = true;
        super.onViewUpdate();
    }

    /** The texture that the sprite is using. */
    get texture()
    {
        return this._texture;
    }

    /**
     * The bounds of the sprite, taking the texture's trim into account.
     * @type {rendering.Bounds}
     */
    get visualBounds()
    {
        updateQuadBounds(this._visualBounds, this._anchor, this._texture, 0);

        return this._visualBounds;
    }

    /**
     * @deprecated
     */
    get sourceBounds()
    {
        deprecation('8.6.1', 'Sprite.sourceBounds is deprecated, use visualBounds instead.');

        return this.visualBounds;
    }

    /** @private */
    protected updateBounds()
    {
        const anchor = this._anchor;
        const texture = this._texture;

        const bounds = this._bounds;

        const { width, height } = texture.orig;

        bounds.minX = -anchor._x * width;
        bounds.maxX = bounds.minX + width;

        bounds.minY = -anchor._y * height;
        bounds.maxY = bounds.minY + height;
    }

Review comment (bug):
```suggestion
        // #if _DEBUG
        deprecation('8.6.1', 'Sprite.sourceBounds is deprecated, use visualBounds instead.');
        // #endif
```


Code after:
    }

    /** The texture that the sprite is using. */
    get texture()
    {
        return this._texture;
    }

    /**
     * The bounds of the sprite, taking the texture's trim into account.
     * @type {rendering.Bounds}
     */
    get visualBounds()
    {
        updateQuadBounds(this._visualBounds, this._anchor, this._texture, 0);

        return this._visualBounds;
    }

    /**
     * @deprecated
     */
    get sourceBounds()
    {
        // #if _DEBUG
        deprecation('8.6.1', 'Sprite.sourceBounds is deprecated, use visualBounds instead.');
        // #endif

        return this.visualBounds;
    }

    /** @private */
    protected updateBounds()
    {
        const anchor = this._anchor;
        const texture = this._texture;

        const bounds = this._bounds;

        const { width, height } = texture.orig;

        bounds.minX = -anchor._x * width;
        bounds.maxX = bounds.minX + width;

        bounds.minY = -anchor._y * height;
        bounds.maxY = bounds.minY + height;
    }

    /**
     * Destroys this sprite renderable and optionally its texture.
     * @param options - Options parameter. A boolean will act as if all options
Repository: pixijs/pixijs
File: tests/visual/scenes/mask/stencil-nested-render-group.scene.ts (TypeScript)

Code before:
/* eslint-disable max-len */
import { AlphaFilter } from '../../../../src/filters';
import { Graphics } from '../../../../src/scene/graphics/shared/Graphics';

import type { Container } from '../../../../src/scene/container/Container';
import type { TestScene } from '../../types';

export const scene: TestScene = {
    it: 'should nested masks in render group correctly',
    create: async (scene: Container) =>
    {
        const g = new Graphics();

        const padding = 20;

        g.rect(0, 0, 128 - padding, 128 - padding);
        g.fill({ color: 0xffff00 });
        g.x = padding / 2;
        g.y = padding / 2;
        scene.addChild(g);

        const mask = new Graphics();

        mask.circle((128 - padding) / 2, (128 - padding) / 2, 50);
        mask.fill({ color: 0xff0000 });
        mask.alpha = 0.5;
        g.addChild(mask);

        g.filters = [new AlphaFilter()];
        g.mask = mask;
        g.eventMode = 'static';

        g.enableRenderGroup();
    },

Review comment (suggestion):
The test case description should be 'should handle nested masks in render group correctly'.
```suggestion
    it: 'should handle nested masks in render group correctly',
```

Code after:
import { AlphaFilter } from '~/filters/defaults/alpha/AlphaFilter';
import { Graphics } from '~/scene/graphics/shared/Graphics';

import type { TestScene } from '../../types';
import type { Container } from '~/scene/container/Container';

export const scene: TestScene = {
    it: 'should handle nested masks in render group correctly',
    create: async (scene: Container) =>
    {
        const g = new Graphics();

        const padding = 20;

        g.rect(0, 0, 128 - padding, 128 - padding);
        g.fill({ color: 0xffff00 });
        g.x = padding / 2;
        g.y = padding / 2;
        scene.addChild(g);

        const mask = new Graphics();

        mask.circle((128 - padding) / 2, (128 - padding) / 2, 50);
        mask.fill({ color: 0xff0000 });
        mask.alpha = 0.5;
        g.addChild(mask);

        g.filters = [new AlphaFilter()];
        g.mask = mask;
        g.eventMode = 'static';

        g.enableRenderGroup();
    },
};
Repository: pixijs/pixijs
File: src/scene/graphics/shared/utils/__tests__/toFillStyle.test.ts (TypeScript)

Code before:
import { Color } from '../../../../../color/Color';
import { Texture } from '../../../../../rendering/renderers/shared/texture/Texture';
import { FillGradient } from '../../fill/FillGradient';
import { FillPattern } from '../../fill/FillPattern';
import { type ConvertedFillStyle, type FillStyle } from '../../FillTypes';
import { toFillStyle } from '../convertFillInputToFillStyle';

describe('toFillStyle', () =>
{
    const defaultStyle: ConvertedFillStyle = {
        color: 0x0,
        alpha: 1,
        texture: null,
        matrix: null,
        textureSpace: null,
        fill: null,
    };

    it('should handle null/undefined values', () =>
    {
        expect(toFillStyle(null, defaultStyle)).toBe(null);
        expect(toFillStyle(undefined, defaultStyle)).toBe(null);
    });

    it('should handle color-like values', () =>
    {
        // Hex number

Review comment (suggestion):
Recommend using the src alias `~`
```suggestion
import { Color } from '~/color/Color';
import { Texture } from '~/rendering/renderers/shared/texture/Texture';
```

Code after:
import { FillGradient } from '../../fill/FillGradient';
import { FillPattern } from '../../fill/FillPattern';
import { type ConvertedFillStyle, type FillStyle } from '../../FillTypes';
import { toFillStyle } from '../convertFillInputToFillStyle';
import { Color } from '~/color/Color';
import { Texture } from '~/rendering/renderers/shared/texture/Texture';

describe('toFillStyle', () =>
{
    const defaultStyle: ConvertedFillStyle = {
        color: 0x0,
        alpha: 1,
        texture: null,
        matrix: null,
        textureSpace: null,
        fill: null,
    };

    it('should handle null/undefined values', () =>
    {
        expect(toFillStyle(null, defaultStyle)).toBe(null);
        expect(toFillStyle(undefined, defaultStyle)).toBe(null);
    });

    it('should handle color-like values', () =>
    {
        // Hex number
Repository: pixijs/pixijs
File: src/utils/__tests__/updateQuadBounds.test.ts (TypeScript)

Code before:
import { type ObservablePoint } from '../../maths/point/ObservablePoint';
import { type Texture } from '../../rendering/renderers/shared/texture/Texture';
import { updateQuadBounds } from '../data/updateQuadBounds';

describe('updateQuadBounds', () =>
{
    let bounds: { minX: number; maxX: number; minY: number; maxY: number };
    let anchor: ObservablePoint;
    let texture: Texture;

    beforeEach(() =>
    {
        // Reset bounds before each test
        bounds = { minX: 0, maxX: 0, minY: 0, maxY: 0 };
        anchor = { _x: 0, _y: 0 } as ObservablePoint;
        texture = {
            orig: { width: 100, height: 100 },
            trim: null,
        } as Texture;
    });

    it('should calculate bounds for untrimmed texture with default anchor (0,0)', () =>
    {
        updateQuadBounds(bounds, anchor, texture);

        expect(bounds.minX).toBe(-0);
        expect(bounds.maxX).toBe(100);

Review comment (suggestion):
```suggestion
import { type ObservablePoint } from '~/maths';
import { type Texture } from '~/rendering';
```

Code after:
import { updateQuadBounds } from '../data/updateQuadBounds';
import { type ObservablePoint } from '~/maths';
import { type Texture } from '~/rendering';

describe('updateQuadBounds', () =>
{
    let bounds: { minX: number; maxX: number; minY: number; maxY: number };
    let anchor: ObservablePoint;
    let texture: Texture;

    beforeEach(() =>
    {
        // Reset bounds before each test
        bounds = { minX: 0, maxX: 0, minY: 0, maxY: 0 };
        anchor = { _x: 0, _y: 0 } as ObservablePoint;
        texture = {
            orig: { width: 100, height: 100 },
            trim: null,
        } as Texture;
    });

    it('should calculate bounds for untrimmed texture with default anchor (0,0)', () =>
    {
        updateQuadBounds(bounds, anchor, texture);

        expect(bounds.minX).toBe(-0);
        expect(bounds.maxX).toBe(100);
Repository: pixijs/pixijs
File: src/scene/text-html/HTMLTextSystem.ts (TypeScript)

Code before:

    public returnTexturePromise(texturePromise: Promise<Texture>)
    {
        texturePromise.then((texture) =>
        {
            this._cleanUp(texture);
        }).catch(() =>
        {
            // #if _DEBUG
            warn('HTMLTextSystem: Failed to clean texture');
            // #endif
        });
    }

    private _cleanUp(texture: Texture)
    {
        TexturePool.returnTexture(texture);
        texture.source.resource = null;
        texture.source.uploadMethodId = 'unknown';
    }

    public destroy()
    {
        // BOOM!
        (this._renderer as null) = null;
    }
}


Review comment (suggestion):
The assignment '(this._renderer as null) = null;' in destroy() is not valid TypeScript syntax and may lead to runtime errors. Consider setting this._renderer = null (if allowed) or implementing proper resource cleanup logic.
```suggestion
        this._renderer = null;
```

Code after:
    }

    public returnTexturePromise(texturePromise: Promise<Texture>)
    {
        texturePromise.then((texture) =>
        {
            this._cleanUp(texture);
        }).catch(() =>
        {
            // #if _DEBUG
            warn('HTMLTextSystem: Failed to clean texture');
            // #endif
        });
    }

    private _cleanUp(texture: Texture)
    {
        TexturePool.returnTexture(texture);
        texture.source.resource = null;
        texture.source.uploadMethodId = 'unknown';
    }

    public destroy()
    {
        // BOOM!
        (this._renderer as null) = null;
    }
}