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

Фронт: Вернуть OffscreenCanvas в цветовой миксер OKLCH #82

Closed
wants to merge 31 commits into from

Conversation

yorgetsuten
Copy link
Contributor

не тестил работу в сафари, но проверка та же что в референсном коммите и на https://sitnik.ru/ru/, если я все правильно понимаю, так что работать должно.

@ai
Copy link
Member

ai commented Feb 28, 2023

не тестил работу в сафари

Проверь что не-воркер рисовка работает поставив туда if (false) вручную

@yorgetsuten
Copy link
Contributor Author

не тестил работу в сафари

Проверь что не-воркер рисовка работает поставив туда if (false) вручную

проверил конечно, работает

@ai
Copy link
Member

ai commented Feb 28, 2023

При запуске в FF у меня такая ошибка нет графиков

SyntaxError: import declarations may only appear at top level of a module

@ai
Copy link
Member

ai commented Feb 28, 2023

В Chrome есть проблема с обновлением графиков:

  1. Берёшь ползунок и начинаешь его медленно двигать не отпуская
  2. Смотришь на связанный с ним график

Сейчас: он обновляется практически в реальном времени (пусть и с низким разрешением).

В PR: он обновляется с большой задержкой или только когда я отпущу ползунок.

@yorgetsuten
Copy link
Contributor Author

В Chrome есть проблема с обновлением графиков:

  1. Берёшь ползунок и начинаешь его медленно двигать не отпуская
  2. Смотришь на связанный с ним график

Сейчас: он обновляется практически в реальном времени (пусть и с низким разрешением).

В PR: он обновляется с большой задержкой или только когда я отпущу ползунок.

локально он у меня обновляется как и должен, в реальном времени и даже разрешение нормальное

@ai
Copy link
Member

ai commented Feb 28, 2023

локально он у меня обновляется как и должен, в реальном времени и даже разрешение нормальное

Может дело в мощности железа (попробуй выставить замедление в DevTools).

Но ещё можем починить в FF и его поведение нам больше подскажет.

Но сначала нужно починить pnpm-лок файл (просто ревертни изменения) и отступы нового файла.

@yorgetsuten
Copy link
Contributor Author

yorgetsuten commented Feb 28, 2023

локально он у меня обновляется как и должен, в реальном времени и даже разрешение нормальное

Может дело в мощности железа (попробуй выставить замедление в DevTools).

Но ещё можем починить в FF и его поведение нам больше подскажет.

Но сначала нужно починить pnpm-лок файл (просто ревертни изменения) и отступы нового файла.

я скопировал pnpm лок с последнего коммита main ветки и отправил в последнем коммите в котором прогнал prettier где только мог
и я не знаю что такое FF, какое полное название я погуглю

@yorgetsuten
Copy link
Contributor Author

локально он у меня обновляется как и должен, в реальном времени и даже разрешение нормальное

Может дело в мощности железа (попробуй выставить замедление в DevTools).

Но ещё можем починить в FF и его поведение нам больше подскажет.

Но сначала нужно починить pnpm-лок файл (просто ревертни изменения) и отступы нового файла.

выставил 6-кратный троттлинг проца, изменений практически нет. справедливости ради проц довольно мощный но все-же..

@ai
Copy link
Member

ai commented Feb 28, 2023

и я не знаю что такое FF, какое полное название я погуглю

Firefox

Я скопировал pnpm лок с последнего коммита main ветки

Может не пушнул? В диффе PR-а есть изменения в pnpm-lock.yaml (и CI из-за этого падает) https://github.com/evilmartians/oklch-picker/pull/82/files#diff-32824c984905bb02bc7ffcef96a77addd1f1602cff71a11fbbfdd7f53ee026bb

прогнал prettier где только мог

Проверь вкладку Files changed в PR, увидишь что файл всё ещё неправильно оформлен https://github.com/evilmartians/oklch-picker/pull/82/files#diff-b0ea296e4ad8fed6150664d32076d99d7c658147a1801d075c47db808a73c305

@yorgetsuten
Copy link
Contributor Author

коммит не пришел, индикации измененных файлов в vs code нет хотя версии pnpm-lock локально и на гитхабе разные. щас придумаю что-нибудь

@yorgetsuten
Copy link
Contributor Author

придумал все пришло

@yorgetsuten
Copy link
Contributor Author

ошибка в ff возможно потому что воркер без type: 'module' только я не знаю как в тс это прописать

@ai
Copy link
Member

ai commented Feb 28, 2023

ошибка в ff возможно потому что воркер без type: 'module' только я не знаю как в тс это прописать

Посмотри как это сделано было в прошлый раз тут (в задаче есть ссылка на коммит)

Ещё надо CI починить (Не забывай вызывать pnpm test перед отправкой PR).

@yorgetsuten

This comment was marked as resolved.

@yorgetsuten
Copy link
Contributor Author

yorgetsuten commented Feb 28, 2023

ошибка в ff возможно потому что воркер без type: 'module' только я не знаю как в тс это прописать

Посмотри как это сделано было в прошлый раз тут (в задаче есть ссылка на коммит)

в отладчике ff воркер уже с type: 'module'

Ещё надо CI починить (Не забывай вызывать pnpm test перед отправкой PR).

All scripts to execute
Package size limit has exceeded by 3.92 kB
Size limit: 60 kB
Size: 63.92 kB

Try to reduce size or increase limit at .size-limit.json
мне повысить лимит?

@yorgetsuten
Copy link
Contributor Author

я залил проект на netlify и все заработало, насколько я понял vite в воркер что-то импортировал в дев билде и ff на это ругался
https://ephemeral-creponne-3cf2a9.netlify.app/#73.4,0.136,141.88,100

@yorgetsuten
Copy link
Contributor Author

yorgetsuten commented Mar 2, 2023

это если что не конечный вариант так что там есть закомментированный код. сейчас я делаю общую очередь и пытаюсь занять воркеры которые сидят без дела, но возникли некоторые сложности а спать тоже нужно

@yorgetsuten
Copy link
Contributor Author

yorgetsuten commented Mar 3, 2023

не знаю каким чудом, но это починило ff. бага с рендером больше нет, версия браузера та же. теперь там общая очередь из которой три воркера берут задачи так что ни один из них больше не сидит без дела. если очередь слишком большая то некоторые задачи выкидываются но наверняка это как-то лучше можно сделать. единственное что теперь канвасы рендерятся с помощью transferFromImageBitmap, а контекст 'bitmaprenderer' поддерживается хуже чем transferControlToOffscreen():
https://caniuse.com/mdn-api_htmlcanvaselement_getcontext_bitmaprenderer_context
https://caniuse.com/?search=transfercontrol
так что может еще одно ветвление if добавить...
ну и еще вот это самое ветвление if (canvasL.transferControlToOffscreen && canvasL.getContext('bitmaprenderer')) теперь довольно большое и очень сильно отличается от else, так что может его в отдельный файл вынести?

@yorgetsuten
Copy link
Contributor Author

yorgetsuten commented Mar 3, 2023

а, и еще неплохо было бы по мере возрастастания нагрузки новые воркеры создавать но не переборщить при этом потоки то не бесконечные так что ну подумаю че с этим делать

@ai
Copy link
Member

ai commented Mar 3, 2023

На ретине выглядит вот так

Captura desde 2023-03-03 12-10-00

canvasHSize: DOMRect
pixelRation: number
}
| {
Copy link
Member

@ai ai Mar 3, 2023

Choose a reason for hiding this comment

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

Сократи этот тип так:

  | {
      type: 'l' | 'c' | 'h'
      isFull: boolean
      c: number
      scale: number
      showP3: boolean
      showRec2020: boolean
      p3: string
      rec2020: string
    }

@ai
Copy link
Member

ai commented Mar 3, 2023

а, и еще неплохо было бы по мере возрастастания нагрузки новые воркеры создавать но не переборщить при этом потоки то не бесконечные так что ну подумаю че с этим делать

Это нельзя сделать. У тебя <canvas>-ом может владеть только один воркер (веб-воркер или главный тред). Поэтому больше 3 воркеров и треда тут не запустить.

@yorgetsuten
Copy link
Contributor Author

а, и еще неплохо было бы по мере возрастастания нагрузки новые воркеры создавать но не переборщить при этом потоки то не бесконечные так что ну подумаю че с этим делать

Это нельзя сделать. У тебя <canvas>-ом может владеть только один воркер (веб-воркер или главный тред). Поэтому больше 3 воркеров и треда тут не запустить.

сейчас ни один воркер не владеет канвасом? если бы кто-то из этих трех им владел то каждый воркер рисовал бы только свой канвас, как было раньше, из-за чего хотя бы один воркер ничего не делал. сейчас воркер просто рисует на каком-то своем внутрененнем независимом offscreencanvas - е, а потом отправляет результат в главный тред который рендерит его на настоящем. это позволяет каждому воркеру не быть привязанным к конкретному канвасу

@ai
Copy link
Member

ai commented Mar 3, 2023

сейчас воркер просто рисует на каком-то своем внутрененнем независимом offscreencanvas

А зачем тогда рисовать по <canvas>, если они не ренедрятся? генерируй тогда просто массив пикселей и возвращай в главный тред (только делай это правильно, чтобы не было копирования массивов, а просто передавалось владение массивом — посмотри API postMessage).

ctxL!.transferFromImageBitmap(e.data.btm)
break
case 'c':
ctxC!.transferFromImageBitmap(e.data.btm)
Copy link
Member

Choose a reason for hiding this comment

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

Время этой операции должно читываться в Freeze. Может оптимизация вообще не происходит, так как эта операция долго работает.

type: 'painted',
renderType: type,
workerType,
btm: canvasC.transferToImageBitmap(),
Copy link
Member

Choose a reason for hiding this comment

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

У тебя тут происходит две лишние операции.

paintLH сначала генерирует ImageBitmap внутри, потом рисует его по невидимому <canvas> воркера, потом ты вытаскиваешь ImageBitmap снова и отправляешь в главный тред. Но зачем, если можно сразу взять ImageBitmap из paintLH без рендера его на <canvas> воркера?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

справедливо

@yorgetsuten
Copy link
Contributor Author

мне не очень нравится то что получилось, вместо двух состояний их шесть и для каждого свой прослушиватель с минимыльным различием в логике. плюс лишнее ветвление в onmessage.
без очереди на слабых устройствах ui пусть и не выглядит резиновым, но очень рваным, так что лично я хотя бы пять последних сообщений но оставлял. по мне так гораздо лучше

@ai
Copy link
Member

ai commented Mar 3, 2023

без очереди на слабых устройствах ui пусть и не выглядит резиновым, но очень рваным

Всё так же не могу проверить из-за того, что сломана поддержка ретины

Captura desde 2023-03-03 15-19-30

canvasL.transferControlToOffscreen &&
canvasL.getContext('bitmaprenderer')
) {
let isBusyL = atom<boolean>(false)
Copy link
Member

Choose a reason for hiding this comment

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

А зачем тут атомы, а не просто boolean? Кажется переусложнение.

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.

Ага, но ты можешь это отслеживание сделать не через стор, а просто через if в коде обработки сообщения.

А-ля (псевдо-код):

worker.onmessage = (e: MessageEvent<MessageData>) => {
  if (e.data.type === 'painted') {
    if (e.data.workerType === 'l') {
      if (lastPendingL) {
        sendMessageL(lastPendingL)
      }
      
    }
  }
)

isFull: boolean,
cb: () => void
isWorker?: boolean
Copy link
Member

Choose a reason for hiding this comment

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

It is dirty. Just call reportFreeze in the place when you call reportPaint without worker.

}

if (renderType === 'l') {
reportFreeze(trackTime(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Ты уверен, что оно работает? Как проверял?

Я сейчас открыл https://ephemeral-creponne-3cf2a9.netlify.app/?bench#67.7,0.136,141.88,100 там сначала показалось 10, а потом резко 0. Меня значение L — снова, сначала 20 потом снова 0.

Кажется сломано, неправильно запоминаются значения (например, два разных блока сообщают цифру и перезаписывают друг друга).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ты уверен, что оно работает? Как проверял?

Я сейчас открыл https://ephemeral-creponne-3cf2a9.netlify.app/?bench#67.7,0.136,141.88,100 там сначала показалось 10, а потом резко 0. Меня значение L — снова, сначала 20 потом снова 0.

Кажется сломано, неправильно запоминаются значения (например, два разных блока сообщают цифру и перезаписывают друг друга).

не работает но не успел исправить, я ушел

@yorgetsuten
Copy link
Contributor Author

без очереди на слабых устройствах ui пусть и не выглядит резиновым, но очень рваным

Всё так же не могу проверить из-за того, что сломана поддержка ретины

Captura desde 2023-03-03 15-19-30

и это тоже не успел..

@ai
Copy link
Member

ai commented Mar 3, 2023

Не продолжай пока работать — что-то у нас с этим подходом код становиться только сложнее, а результат отрисовки хуже.

Я на основе твоей идеи с ImageBitmap сейчас придумаю новое задание.

Прошлое с OffscreenCanvas отмечено как решенное тобой https://cultofmartians.com/tasks/oklch-offscreen-canvas.html#task

Новое опубликую сегодня/завтра.

@ai ai closed this Mar 3, 2023
@ai
Copy link
Member

ai commented Mar 3, 2023

Вот новое план (в этот раз никаких 2-х блоков для Сафари и остальных, и без scale-хака) https://cultofmartians.com/tasks/oklchcpc-parallel.html#task

@f3d0t
Copy link
Contributor

f3d0t commented Mar 3, 2023

Тоже работал над данной задачей, столкнулся с абсолютно такой же проблемой мерцания в FF, боролся с ней достаточно долго, но решить, увы, не удалось
Причем эта проблема, похоже, существовала и во время референсного коммита, просто она не была так заметна, тк отрисовка производилась по клику , а не по ведению.
Что пробовал чтобы пофиксить:

  • убрать динамический скейлинг - мерцать перестает при постоянном scale равном 4
  • обернуть в requestAnimationFrame как отрисовку в воркере, так и постановку задач в setComponentsFromSpace в paint/index.ts
  • очередь тасок в воркеры
  • убрать изменение размеров канваса в setScale

@ai
Copy link
Member

ai commented Mar 3, 2023

@f3d0t мне кажется что если взять ImageBitmap и параллельную отрисовку одного графика на нескольких воркерах, то можно вообще выбросить scale-хак и упростить код. Заодно не надо будет старый Сафари поддерживать (где нет OffscreenCanvas, который не нужен с ImageBitmap-хаком).

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.

4 participants