ronantakizawa/codereview-bench huggingface dataset:
Repository: pixijs/pixijs
File: src/rendering/renderers/shared/system/SharedSystems.ts (TypeScript)

Code before:
     *
     * e.g.
     * ```js
     * import 'accessibility';
     * import 'app';
     * import 'events';
     * import 'spritesheet';
     * import 'graphics';
     * import 'mesh';
     * import 'text';
     * import 'text-bitmap';
     * import 'text-html';
     * import { autoDetectRenderer } from 'pixi.js';
     *
     * const renderer = await autoDetectRenderer({
     *   width: 800,
     *   height: 600,
     *   manualExtensions: true,
     * });
     * ```
     * @default false
     */
    manualExtensions?: boolean;
    /**
     * @default true
     * @deprecated Use `manualExtensions` instead
     */
    manageImports?: boolean;
}


Review comment (suggestion):
This probably should be a version, and use the `@see` tag to reference the new API.

Code after:
     *
     * e.g.
     * ```js
     * import 'accessibility';
     * import 'app';
     * import 'events';
     * import 'spritesheet';
     * import 'graphics';
     * import 'mesh';
     * import 'text';
     * import 'text-bitmap';
     * import 'text-html';
     * import { autoDetectRenderer } from 'pixi.js';
     *
     * const renderer = await autoDetectRenderer({
     *   width: 800,
     *   height: 600,
     *   skipExtensionImports: true,
     * });
     * ```
     * @default false
     */
    skipExtensionImports?: boolean;
    /**
     * @default true
     * @deprecated since 8.1.6
     * @see `skipExtensionImports`
     */
    manageImports?: boolean;
}
Repository: pixijs/pixijs
File: tests/renderering/mesh/Mesh.test.ts (TypeScript)

Code before:

        const gpuMesh = renderer.renderPipes.mesh['_gpuBatchableMeshHash'][mesh.uid];

        expect(gpuMesh).toBeUndefined();

        mesh.destroy();

        expect(mesh.geometry).toBeNull();
        expect(mesh.texture).toBeNull();

        expect(renderer.renderPipes.mesh['_meshDataHash'][mesh.uid]).toBeNull();
        expect(renderer.renderPipes.mesh['_gpuBatchableMeshHash'][mesh.uid]).toBeUndefined();
    });

    it('should support color tinting', () =>
    {
        const mesh = getMesh();

        mesh.tint = 'red';

        expect(mesh.tint).toBe(0xff0000);

        mesh.destroy();
    });

    it.only('should have correct blendMode based on textures alpha status when not batching', async () =>
    {
        const renderer = await getWebGLRenderer();

        const container = new Container();

        const mesh = getMesh();

        mesh.geometry.batchMode = 'no-batch';
        mesh.texture.source.alphaMode = 'no-premultiply-alpha';

        container.addChild(mesh);

        // will assign the state...
        renderer.render({ container });

        expect(mesh.state.blendMode).toBe('normal-npm');

        mesh.destroy();
    });
});


Review comment (performance):
```suggestion
    it('should have correct blendMode based on textures alpha status when not batching', async () =>
```

Code after:

        const gpuMesh = renderer.renderPipes.mesh['_gpuBatchableMeshHash'][mesh.uid];

        expect(gpuMesh).toBeUndefined();

        mesh.destroy();

        expect(mesh.geometry).toBeNull();
        expect(mesh.texture).toBeNull();

        expect(renderer.renderPipes.mesh['_meshDataHash'][mesh.uid]).toBeNull();
        expect(renderer.renderPipes.mesh['_gpuBatchableMeshHash'][mesh.uid]).toBeUndefined();
    });

    it('should support color tinting', () =>
    {
        const mesh = getMesh();

        mesh.tint = 'red';

        expect(mesh.tint).toBe(0xff0000);

        mesh.destroy();
    });

    it('should have correct blendMode based on textures alpha status when not batching', async () =>
    {
        const renderer = await getWebGLRenderer();

        const container = new Container();

        const mesh = getMesh();

        mesh.geometry.batchMode = 'no-batch';
        mesh.texture.source.alphaMode = 'no-premultiply-alpha';

        container.addChild(mesh);

        // will assign the state...
        renderer.render({ container });

        expect(mesh.state.blendMode).toBe('normal-npm');

        mesh.destroy();
    });
});
Repository: pixijs/pixijs
File: src/scene/text-html/HTMLTextPipe.ts (TypeScript)

Code before:
    } as const;

    private _renderer: Renderer;

    private _gpuText: Record<number, {
        textureNeedsUploading: boolean;
        generatingTexture: boolean;
        texture: Texture,
        currentKey: string,
        batchableSprite: BatchableSprite,
    }> = Object.create(null);

    constructor(renderer: Renderer)
    {
        this._renderer = renderer;
        this._renderer.runners.resolutionChange.add(this);
    }

    public resolutionChange()
    {
        for (const i in this._gpuText)
        {
            const gpuText = this._gpuText[i];
            const text = gpuText.batchableSprite.renderable as HTMLText;

            text._resolution = text._autoResolution ? this._renderer.resolution : text.resolution;
            text.onViewUpdate();
        }
    }

    public validateRenderable(htmlText: HTMLText): boolean
    {
        const gpuText = this._getGpuText(htmlText);

        const newKey = htmlText._getKey();

        if (gpuText.textureNeedsUploading)
        {
            gpuText.textureNeedsUploading = false;

            return true;
        }

        if (gpuText.currentKey !== newKey)
        {
            // TODO - could look into optimising this a tad!
            // if its a single texture, then we could just swap it?
            // same for CanvasText..
            return true;
        }


Review comment (suggestion):
mini optimisation - we only need to update the ones with text._autoResolution set to true.

```
if(text._autoResolution)
{
    // will trigger on viewUpdate
    text.resolution = this.renderer.resolution
}
```


Code after:
    } as const;

    private _renderer: Renderer;

    private _gpuText: Record<number, {
        textureNeedsUploading: boolean;
        generatingTexture: boolean;
        texture: Texture,
        currentKey: string,
        batchableSprite: BatchableSprite,
    }> = Object.create(null);

    constructor(renderer: Renderer)
    {
        this._renderer = renderer;
        this._renderer.runners.resolutionChange.add(this);
    }

    public resolutionChange()
    {
        for (const i in this._gpuText)
        {
            const gpuText = this._gpuText[i];
            const text = gpuText.batchableSprite.renderable as HTMLText;

            if (text._autoResolution)
            {
                text._resolution = this._renderer.resolution;
                text.onViewUpdate();
            }
        }
    }

    public validateRenderable(htmlText: HTMLText): boolean
    {
        const gpuText = this._getGpuText(htmlText);

        const newKey = htmlText._getKey();

        if (gpuText.textureNeedsUploading)
        {
            gpuText.textureNeedsUploading = false;

            return true;
        }

        if (gpuText.currentKey !== newKey)
        {
            // TODO - could look into optimising this a tad!
            // if its a single texture, then we could just swap it?
            // same for CanvasText..
Repository: pixijs/pixijs
File: src/filters/Filter.ts (TypeScript)

Code before:
     * @default 0
     */
    public padding: number;

    /**
     * should the filter use antialiasing?
     * @default inherit
     */
    public antialias: FilterAntialias;

    /** If enabled is true the filter is applied, if false it will not. */
    public enabled = true;

    /**
     * The gpu state the filter requires to render.
     * @internal
     * @ignore
     */
    public _state = State.for2d();

    /**
     * The resolution of the filter. Setting this to be lower will lower the quality but
     * increase the performance of the filter.
     * @default 1
     */
    public resolution: number | null;

    /**
     * Whether or not this filter requires the previous render texture for blending.
     * @default false
     */
    public blendRequired: boolean;

    /**
     * @param options - The optional parameters of this filter.
     */
    constructor(options: FilterWithShader)
    {
        options = { ...Filter.defaultOptions, ...options };

        super(options as ShaderWithResources);

        this.padding = options.padding;

        // check if is boolean
        if (typeof options.antialias === 'boolean')
        {
            this.antialias = options.antialias ? 'on' : 'off';
        }
        else
        {

Review comment (suggestion):
```suggestion
    public resolution: number | 'inherit';
```


Code after:
     * @default 0
     */
    public padding: number;

    /**
     * should the filter use antialiasing?
     * @default inherit
     */
    public antialias: FilterAntialias;

    /** If enabled is true the filter is applied, if false it will not. */
    public enabled = true;

    /**
     * The gpu state the filter requires to render.
     * @internal
     * @ignore
     */
    public _state = State.for2d();

    /**
     * The resolution of the filter. Setting this to be lower will lower the quality but
     * increase the performance of the filter.
     * @default 1
     */
    public resolution: number | 'inherit';

    /**
     * Whether or not this filter requires the previous render texture for blending.
     * @default false
     */
    public blendRequired: boolean;

    /**
     * @param options - The optional parameters of this filter.
     */
    constructor(options: FilterWithShader)
    {
        options = { ...Filter.defaultOptions, ...options };

        super(options as ShaderWithResources);

        this.blendMode = options.blendMode;
        this.padding = options.padding;

        // check if is boolean
        if (typeof options.antialias === 'boolean')
        {
            this.antialias = options.antialias ? 'on' : 'off';
        }
        else
Repository: pixijs/pixijs
File: src/rendering/renderers/gpu/shader/GpuShaderSystem.ts (TypeScript)

Code before:

/**
 * A system that manages the rendering of GpuPrograms.
 * @memberof rendering
 */
export class GpuShaderSystem implements ShaderSystem
{
    /** @ignore */
    public static extension = {
        type: [
            ExtensionType.WebGPUSystem,
        ],
        name: 'shader',
    } as const;

    public maxTextures: number;

    private _gpu: GPU;

    private readonly _gpuProgramData: Record<number, GPUProgramData> = Object.create(null);

    protected contextChange(gpu: GPU): void
    {
        this._gpu = gpu;

        this.maxTextures = getMaxTexturesPerBatch();
    }

    public getProgramData(program: GpuProgram)
    {
        return this._gpuProgramData[program._layoutKey] || this._createGPUProgramData(program);
    }

    private _createGPUProgramData(program: GpuProgram)
    {
        const device = this._gpu.device;

        const bindGroups = program.gpuLayout.map((group) => device.createBindGroupLayout({ entries: group }));

        const pipelineLayoutDesc = { bindGroupLayouts: bindGroups };

        this._gpuProgramData[program._layoutKey] = {
            bindGroups,
            pipeline: device.createPipelineLayout(pipelineLayoutDesc),
        };

        // generally we avoid having to make this automatically
        // keeping this for a reminder, if any issues popup
        // program._gpuLayout = {
        //     bindGroups: null,
        //     pipeline: 'auto',

Review comment (suggestion):
I suppose we could use webgpu here to determine the maximum textures given the webgpu context. But I don't know how that would work.

Code after:
/**
 * A system that manages the rendering of GpuPrograms.
 * @memberof rendering
 */
export class GpuShaderSystem implements ShaderSystem
{
    /** @ignore */
    public static extension = {
        type: [
            ExtensionType.WebGPUSystem,
        ],
        name: 'shader',
    } as const;

    public maxTextures: number;

    private _gpu: GPU;

    private readonly _gpuProgramData: Record<number, GPUProgramData> = Object.create(null);

    protected contextChange(gpu: GPU): void
    {
        this._gpu = gpu;

        this.maxTextures = gpu.device.limits.maxSampledTexturesPerShaderStage;
    }

    public getProgramData(program: GpuProgram)
    {
        return this._gpuProgramData[program._layoutKey] || this._createGPUProgramData(program);
    }

    private _createGPUProgramData(program: GpuProgram)
    {
        const device = this._gpu.device;

        const bindGroups = program.gpuLayout.map((group) => device.createBindGroupLayout({ entries: group }));

        const pipelineLayoutDesc = { bindGroupLayouts: bindGroups };

        this._gpuProgramData[program._layoutKey] = {
            bindGroups,
            pipeline: device.createPipelineLayout(pipelineLayoutDesc),
        };

        // generally we avoid having to make this automatically
        // keeping this for a reminder, if any issues popup
        // program._gpuLayout = {
        //     bindGroups: null,
        //     pipeline: 'auto',
        // };
Repository: pixijs/pixijs
File: src/rendering/renderers/shared/buffer/Buffer.ts (TypeScript)

Code before:
     * @internal
     * @ignore
     */
    public _updateSize: number;

    private _data: TypedArray;

    private _dataInt32: Int32Array = null;

    /**
     * should the GPU buffer be shrunk when the data becomes smaller?
     * changing this will cause the buffer to be destroyed and a new one created on the GPU
     * this can be expensive, especially if the buffer is already big enough!
     * setting this to false will prevent the buffer from being shrunk. This will yield better performance
     * if you are constantly setting data that is changing size often.
     * @default true
     */
    public shrinkToFit = true;

    /**
     * Has the buffer been destroyed?
     * @readonly
     */
    public destroyed = false;

    public _lastBindBaseLocation: number = -1;
    public _lastBindCallId: number = -1;

    /**
     * Creates a new Buffer with the given options
     * @param options - the options for the buffer
     */
    constructor(options: BufferOptions)
    {
        let { data, size } = options;
        const { usage, label, shrinkToFit } = options;

        super();

        if (data instanceof Array)
        {
            data = new Float32Array(data as number[]);
        }

        this._data = data as TypedArray;

        size = size ?? (data as TypedArray)?.byteLength;

        const mappedAtCreation = !!data;

        this.descriptor = {

Review comment (suggestion):
Lets stick `_lastBindBaseLocation` and `_lastBindCallId` on `GLBuffer` class as they are WebGL specific (and internal)

Code after:
     * @internal
     * @ignore
     */
    public _updateSize: number;

    private _data: TypedArray;

    private _dataInt32: Int32Array = null;

    /**
     * should the GPU buffer be shrunk when the data becomes smaller?
     * changing this will cause the buffer to be destroyed and a new one created on the GPU
     * this can be expensive, especially if the buffer is already big enough!
     * setting this to false will prevent the buffer from being shrunk. This will yield better performance
     * if you are constantly setting data that is changing size often.
     * @default true
     */
    public shrinkToFit = true;

    /**
     * Has the buffer been destroyed?
     * @readonly
     */
    public destroyed = false;

    /**
     * Creates a new Buffer with the given options
     * @param options - the options for the buffer
     */
    constructor(options: BufferOptions)
    {
        let { data, size } = options;
        const { usage, label, shrinkToFit } = options;

        super();

        if (data instanceof Array)
        {
            data = new Float32Array(data as number[]);
        }

        this._data = data as TypedArray;

        size = size ?? (data as TypedArray)?.byteLength;

        const mappedAtCreation = !!data;

        this.descriptor = {
            size,
            usage,
            mappedAtCreation,
Repository: pixijs/pixijs
File: src/scene/container/Container.ts (TypeScript)

Code before:
     * A value that increments each time the containe is modified
     * eg children added, removed etc
     * @ignore
     */
    public _didContainerChangeTick = 0;
    /**
     * A value that increments each time the container view is modified
     * eg texture swap, geometry change etc
     * @ignore
     */
    public _didViewChangeTick = 0;

    /**
     * We now use the _didContainerChangeTick and _didViewChangeTick to track changes
     * @deprecated since 8.2.6
     * @ignore
     */
    set _didChangeId(value: number)
    {
        this._didViewChangeTick = (value >> 12) & 0xFFF; // Extract the upper 12 bits
        this._didContainerChangeTick = value & 0xFFF; // Extract the lower 12 bits
    }

    get _didChangeId(): number
    {
        return (this._didContainerChangeTick % 0xFFF) | ((this._didViewChangeTick % 0xFFF) << 12);
    }

    /**
     * property that tracks if the container transform has changed
     * @ignore
     */
    private _didLocalTransformChangeId = -1;

    constructor(options: ContainerOptions<C> = {})
    {
        super();

        assignWithIgnore(this, options, {
            children: true,
            parent: true,
            effects: true,
        });

        options.children?.forEach((child) => this.addChild(child));
        this.effects = [];
        options.parent?.addChild(this);
    }

    /**
     * Adds one or more children to the container.

Review comment (suggestion):
```suggestion
        return (this._didContainerChangeTick & 0xFFF) | ((this._didViewChangeTick & 0xFFF) << 12);
```

Code after:
     * A value that increments each time the containe is modified
     * eg children added, removed etc
     * @ignore
     */
    public _didContainerChangeTick = 0;
    /**
     * A value that increments each time the container view is modified
     * eg texture swap, geometry change etc
     * @ignore
     */
    public _didViewChangeTick = 0;

    /**
     * We now use the _didContainerChangeTick and _didViewChangeTick to track changes
     * @deprecated since 8.2.6
     * @ignore
     */
    set _didChangeId(value: number)
    {
        this._didViewChangeTick = (value >> 12) & 0xFFF; // Extract the upper 12 bits
        this._didContainerChangeTick = value & 0xFFF; // Extract the lower 12 bits
    }

    get _didChangeId(): number
    {
        return (this._didContainerChangeTick & 0xfff) | ((this._didViewChangeTick & 0xfff) << 12);
    }

    /**
     * property that tracks if the container transform has changed
     * @ignore
     */
    private _didLocalTransformChangeId = -1;

    constructor(options: ContainerOptions<C> = {})
    {
        super();

        assignWithIgnore(this, options, {
            children: true,
            parent: true,
            effects: true,
        });

        options.children?.forEach((child) => this.addChild(child));
        this.effects = [];
        options.parent?.addChild(this);
    }

    /**
     * Adds one or more children to the container.
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';

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.uid & 255) << 24)
            | ((child._didViewChangeTick % 0xfff) << 12)
            | (child._didContainerChangeTick % 0xfff);

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

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

        checkChildrenDidChange(container, previousData);

        expect(previousData).toEqual({
            data: [childChange],
            index: 1,
            didChange: false,
        });

        child.x += 1;

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

        checkChildrenDidChange(container, previousData);


Review comment (suggestion):
```suggestion
            | ((child._didViewChangeTick & 0xfff) << 12)
            | (child._didContainerChangeTick & 0xfff);
```

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

        child.x += 1;

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

        checkChildrenDidChange(container, previousData);

        childChange = ((child._didViewChangeTick & 0xffff) << 16) | (child._didContainerChangeTick & 0xffff);
Repository: pixijs/pixijs
File: tests/scene/checkChildrenDidChange.tests.ts (TypeScript)

Code before:
            data: [childChange],
            index: 1,
            didChange: true,
        });

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

        checkChildrenDidChange(container, previousData);

        expect(previousData).toEqual({
            data: [childChange],
            index: 1,
            didChange: false,
        });

        child.x += 1;

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

        checkChildrenDidChange(container, previousData);

        childChange = ((child.uid & 255) << 24)
        | ((child._didViewChangeTick % 0xfff) << 12)
        | (child._didContainerChangeTick % 0xfff);

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


Review comment (suggestion):
```suggestion
        | ((child._didViewChangeTick & 0xfff) << 12)
        | (child._didContainerChangeTick & 0xfff);
```

Code after:
            didChange: true,
        });

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

        checkChildrenDidChange(container, previousData);

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

        child.x += 1;

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

        checkChildrenDidChange(container, previousData);

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

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