Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parallel rendering #84

Merged
merged 52 commits into from
Mar 13, 2023
Merged

Add parallel rendering #84

merged 52 commits into from
Mar 13, 2023

Conversation

yorgetsuten
Copy link
Contributor

в сафари и ff работает, багов нет. единственное что в paintSeparatorPixel() как-то неправильно отрабатывает альфа канал. вместо прозрачности у линий как-будто инвертируется фоновый черный цвет

lib/canvas.ts Outdated
@@ -10,10 +10,10 @@ export function getCleanCtx(
return ctx
}

let originSize = new Map<HTMLCanvasElement, [number, number]>()
export let originSize = new Map<HTMLCanvasElement, [number, number]>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А зачем экспорт? Этот Map вроде был нужен для понижения разрешения и сейчас больше не нужен.

lib/paint.ts Outdated
color: string
): void {
let pos = 4 * (y * pixels.width + x)
let colorArr = color
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Такой обратный парсинг — грязно. Лучше измени paintSeparator, чтобы там нормально конвертировать цвет в RGB и тогда не нужно будет и этой функции.

lib/paint.ts Outdated
pixels.data[pos + 3] = +colorArr[3] * 255
}

export function trackTime(cb: () => void): number {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trackTime не имеет отношения к рисованию, чтобы быть тут. Она по сути же может замерять что угодно. Создай lib/time.js.

pnpm-lock.yaml Outdated
@@ -1,4 +1,4 @@
lockfileVersion: 5.4
lockfileVersion: 5.3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Обнови pnpm и откати изменение файла

@@ -1,12 +1,12 @@
import { atom, map } from 'nanostores'
import { setStart, resetFreeze, reportFreeze } from './benchmark.js'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESLint тут ругается, потому что у меня импорты группируются — из npm идут отдельным блоком, из проекта — другим.

Перенеси этот импорт в группу ниже.


for (let i = 0; i < availableWorkers; i++) {
let renderType: RenderType =
canvas === canvasL ? 'l' : canvas === canvasC ? 'c' : 'h'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вложенные a ? b : c — плохая практика (мозг ломается при попытке такое прочитать). ESLint тут тебе и кидает поэтому предупреждение.

Переделай на обычные if.

Copy link
Contributor Author

@yorgetsuten yorgetsuten Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

у if своя область видимости и я подумал что это немного перегрузит функцию поэтому просто в параметр перекинул

@@ -5,86 +5,37 @@ import { benchmarking } from './url.js'

export type RenderType = 'l' | 'c' | 'h'

export let lastBenchmark = map({ freeze: 0, quick: 0, full: 0 })
export let lastBenchmark = map({ freezeSum: 0, freezeMax: 0, frame: 0, full: 0 })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full непонятно о чём речь.

Давай переименуем в framepaint и fullworkersSum. И так же во всех функциях и в UI.

color: string,
line: [number, number][] | undefined
): void {
let colorArr = color
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прошлый комментарий был про то, что парсить rgb() — нельзя. Зачем перенёс просто сюда?

Я предлагаю отпарсить цвет нормально вызвав rgb(string) и получив нормальные цифры. Только сделай это один раз, там где достаются цвета разделителя.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а что за rgb(string)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ладно я нашел какую-то регулярку надеюсь так можно...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rgb() из lib/colors.js может парсить rgb()

y: number,
colorArr: string[]
): void {
let pos = 4 * (y * pixels.width + x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сейчас paintSeparatorPixel стала слишком маленькой, переноси код в paintSeperator

count: 0,
total: 0,
prevScale: 1
export function setStart(ms: number): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не понятно, что за start. Давай, например, setFrameStart(ms).

quick[type].prevScale = scale
return scale
export function resetFreeze(): void {
lastBenchmark.get().freezeMax = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Значения можно менять только через setKey()

return scale
export function resetFreeze(): void {
lastBenchmark.get().freezeMax = 0
lastBenchmark.get().freezeSum = 0
}

export function reportFull(ms: number): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У тебя одно и то же имя аргумента используется и для продолжительности и для времени.

Давай тут использовать time (и так же в setFrameStart).

@@ -93,25 +44,8 @@ const WORST_HUE = 40
const MAX_TIME = 300

export function getLastBenchmarkColor(): string {
let { freeze } = lastBenchmark.get()
let hue = BEST_HUE - ((BEST_HUE - WORST_HUE) * freeze) / MAX_TIME
let hue = BEST_HUE - ((BEST_HUE - WORST_HUE) * lastBenchmark.get().freezeSum) / MAX_TIME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Строка длиннее 80 символов (и кажется ты не везде Prettier запускаешь).

Верни подход, что мы достаём переменную на отдельной строчке.

runListeners(changeListeners, prev)
}
})
export let framesToChange = atom<number>(0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Передавай framesToChange как обычное число в аргументы listener. Типа i.l(value.l, framesToChange).

Это будет проще, чем заводить отдельный стор. Всё равно тебе framesToChange нужен в том же listener.

types.d.ts Outdated
message: object,
transfer: (Transferable | HTMLCanvasElement)[]
): void
postMessage(message: object, transfer: Transferable[]): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай вернём старый тип, он правильнее и может в будущем пригодиться

types.d.ts Outdated
@@ -121,7 +120,16 @@ declare module 'culori/fn' {
export function formatRgb(c: Color): string
export function formatRgb(c: string): string | undefined

export type Color = Hsl | Lab | Lch | Oklab | Oklch | Xyz65 | P3 | Rec2020 | Rgb
export type Color =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Откати (кажется у тебя старый Prettier)

= "Freeze     "
strong.benchmark_freeze
= "Freeze sum  "
strong.benchmark_freeze_sum
Copy link
Member

@ai ai Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У меня система именования CSS-классов вида .block-name_element-name. Тут должно быть benchmark_freeze-sum.

@@ -0,0 +1,100 @@
import type { RenderType } from '../../stores/benchmark'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import type { RenderType } from '../../stores/benchmark'
import type { RenderType } from '../../stores/benchmark.js'

На конце импортов по правилам ESM всегда должно быть расширение. Ниже так же исправь (и проверь остальные импорты).

let unbusyWorkers: Worker[] = []
let busyWorkers: Worker[] = []

let totalWorkers = navigator.hardwareConcurrency
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лучше написать так let totalWorkers = navigator.hardwareConcurrency ?? 12

?? делает то же, только короче и чуть правильнее (проверяет что это именно undefined, а не false).

Ну и 12 воркеров мало у кого сейчас есть. Давай тут хотя бы 8.

...busyWorkers.splice(busyWorkers.indexOf(worker), 1)
]

if (e.data.renderType === 'l') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Логика для l, c и h очень похожая. Надо её куда-то вынести.

let ctx = getCleanCtx(canvasC)

;[...pixelsC, e.data].forEach(pixels => {
let { pixelsBuffer, pixelsWidth, pixelsHeight, xPos } = pixels
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А зачем тут так? Если ниже писать pixelsBuffer вместо pixels.pixelsBuffer то будет не сложнее.

worker.onmessage = (e: MessageEvent<PaintedMessageData>) => {
let start = Date.now()

unbusyWorkers = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Очень сложно читать. Давай разделим доставание из одного массива и помещение в другой массив.

availableWorkers: number,
renderType: RenderType,
canvas: HTMLCanvasElement,
lch: number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lch название для LCH-цвета. Давай лучше value.

@ai ai merged commit e5349be into evilmartians:v1.1 Mar 13, 2023
ai added a commit that referenced this pull request Mar 14, 2023
* move drawing to OffscreenCanvas

* correct quotes and tabs

* prettier

* prettier

* prettier

* pnpm lock

* fix errors

* fix errors

* increase size limit

* fix types

* fix package

* fix types

* fix types

* fix types

* test without worker

* test with worker

* fix eslint

* fix eslint

* Update .size-limit.json

Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>

* fix reportFreeze

* fix dynamic scaling

* ifx types

* fix

* .

* add job queue

* report paint

* general queue

* no queue

* report freeze

* add parallel rendering

* benchmark

* clg

* clg

* retina

* Revert "clg"

* only last pending test

* no retina

* hardwareConcurrency

* fix

* shared workers

* fix

* fix eslint

* fix

* fix

* fix

* rename benchmark

---------

Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants