evilmartians / oklch-picker

Color Picker for LCH
https://oklch.com
Other
849 stars 65 forks source link

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

Closed yorgetsuten closed 1 year ago

yorgetsuten commented 1 year ago

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

ai commented 1 year ago

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

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

yorgetsuten commented 1 year ago

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

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

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

ai commented 1 year ago

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

SyntaxError: import declarations may only appear at top level of a module
ai commented 1 year ago

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

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

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

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

yorgetsuten commented 1 year ago

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

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

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

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

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

ai commented 1 year ago

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

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

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

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

yorgetsuten commented 1 year ago

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

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

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

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

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

yorgetsuten commented 1 year ago

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

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

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

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

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

ai commented 1 year ago

и я не знаю что такое 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 commented 1 year ago

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

yorgetsuten commented 1 year ago

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

yorgetsuten commented 1 year ago

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

ai commented 1 year ago

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

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

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

yorgetsuten commented 1 year ago

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

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

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

types.d.ts:3:3 - error TS2687: All declarations of 'transferControlToOffscreen' must have identical modifiers. 3 transferControlToOffscreen?: () => OffscreenCanvas декларация transferControlToOffscreen всего одна

yorgetsuten commented 1 year ago

ошибка в 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 commented 1 year ago

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

yorgetsuten commented 1 year ago

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

ai commented 1 year ago

мне повысить лимит?

Ага

возможно какая-то особенность работы воркеров в ff, было такое в прошлый раз?

Неа, в прошлый раз всё работало одинаково. Надо дебажить.

ai commented 1 year ago

Всё ещё проблемы с типами.

lib/time.ts(6,18): error TS2503: Cannot find namespace 'NodeJS'.
stores/benchmark.ts(27,24): error TS2503: Cannot find namespace 'NodeJS'.

Вызывай pnpm test перед тем как пушить в репо.

yorgetsuten commented 1 year ago

Всё ещё проблемы с типами.

lib/time.ts(6,18): error TS2503: Cannot find namespace 'NodeJS'.
stores/benchmark.ts(27,24): error TS2503: Cannot find namespace 'NodeJS'.

Вызывай pnpm test перед тем как пушить в репо.

тест доходит до сборки и крашится с этим "LCH" не является внутренней или внешней командой, исполняемой программой или пакетным файлом.  ERROR  Test failed. See above for more details. все ошибки с типами которые он мне давал я пофиксил если как и было поставить типы number, то:

stores/benchmark.ts:35:5 - error TS2322: Type 'Timeout' is not assignable to type 'number'. 35 collectingTimeout = setTimeout(() => {

ai commented 1 year ago

"LCH" не является внутренней или внешней

У тебя Винда? Можешь убрать LCH=1 pnpm build из test перед запуском (или попробовать починить для винды)

yorgetsuten commented 1 year ago

"LCH" не является внутренней или внешней

У тебя Винда? Можешь убрать LCH=1 pnpm build из test перед запуском (или попробовать починить для винды)

убрал, про ноду даже не заикнулся, ошибок нет попробую аннотировать в types.d.ts

yorgetsuten commented 1 year ago

да что за анекдот такой с типами

yorgetsuten commented 1 year ago

вернул таймаутам типы number, локально тесты упали но может хоть так тут проканает

yorgetsuten commented 1 year ago

поменял canvasL.transferControlToOffscreen на false в условии, в ff рендер без воркера работает без багов. логика та же

yorgetsuten commented 1 year ago

в edge тоже никаких проблем нет, в общем, это видимо реально какой-то баг ff. в какой-то обнове поломали воркеры или offscreencanvas

ai commented 1 year ago

Отлично, типы починились (у тебя может быть другая версия TypeScript в IDE, а может Windows влияет).

Теперь надо ESLint починить (у меня перевод строки между импортами типов, внешними импортами и локальными импортами).

В FF я тогда чуть позже подебажу сам.

SparLaimor commented 1 year ago

In this implementation, the logic of reportFreeze changes. Because messages from the worker come after the end of calling runListeners.

yorgetsuten commented 1 year ago

Because messages from the worker come after the end of calling runListeners.

i don't think so. reportFreeze only sets the state to a value that does not depend on the time the message came

SparLaimor commented 1 year ago

Because messages from the worker come after the end of calling runListeners.

i don't think so. reportFreeze only sets the state to a value that does not depend on the time the message came

reportFreeze sets the execution time of each render and at the end sets the value of runListeners execution time. When doing this sequentially, there will always be the sum of the time of all listeners (last call in runListeners). But with a worker, the sequence can change.

ai commented 1 year ago
reportPaint(e.data.renderType, e.data.ms, e.data.isFull)

@sxdav I afraid @SparLaimor is right. Freeze in benchmark shows long main thread was blocked. After adding OffscreenCanvas it should be reduced. In Chrome, I see 22 ms on L changes in Freeze, which a lot (the current app has the same).

You should use e.data.ms for Pre-paint and Full paint only. Freeze should be tracked in main thread (inside onPaint call).

ai commented 1 year ago

У меня всё ещё эта ошибка при запуске pnpm start в Firefox

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

Её надо исправить, иначе я не смогу разрабатывать проект дальше (FF мой основной браузер).

SparLaimor commented 1 year ago

There is another problem that is reproduced better on workers. The scale formula is too aggressive with a small number of measurements. If the time of the first renderings was too long, then the scale is limited to MAX_SCALE and then scale->+∞. After that, dynamic scaling is disabled, even if the performance allows using average scale values.

Therefore, situations may arise:

  1. Show Rec2020 enabled. The user performs a couple of actions. MAX_SCALE has been reached. Switch Show Rec2020. Dynamic rendering will not work even if the measurements are good.
  2. Show Rec2020 disabled. The user performs a couple of actions. MAX_SCALE has not been reached. Switch Show Rec2020. Dynamic rendering works because the measurements have stabilized.
ai commented 1 year ago

Show Rec2020 enabled. The user performs a couple of actions. MAX_SCALE has been reached. Switch Show Rec2020. Dynamic rendering will not work even if the measurements are good.

We can clean scale after changing the settings (since anyway we need to recalc performance metrics).

the scale is limited to MAX_SCALE and then scale->+∞

Why it goes to ∞? I think it should keep at MAX_SCALE.

SparLaimor commented 1 year ago

Why it goes to ∞? I think it should keep at MAX_SCALE.

Because reportQuick is no longer called. total/count becomes a constant. And the only value that is recalculated is prevScale.

Perhaps not too accurately, but it reflects that the scale is growing

let scales = [2]
let timeAfterLimit;

for (let i = 0; i < 10; ++i) {
  let time = timeAfterLimit || 44 / scales.length  // 44ms - 0ms - 0ms - 0ms ...
  scales.unshift(Math.ceil((scales[0] * time) / 16))

  if (scales[0] > 8) timeAfterLimit = time
}

scales
ai commented 1 year ago

Because reportQuick is no longer called

@sxdav we need to bring it back too of course

SparLaimor commented 1 year ago

Because reportQuick is no longer called

@sxdav we need to bring it back too of course

This is not a problem in the current PR

let scale = getQuickScale('c', isFull) // prevScale grows
if (scale > MAX_SCALE) return //  reportQuick is no called

But due to the fact that the worker renders slower at the beginning, this problem is easier to identify.

yorgetsuten commented 1 year ago

У меня всё ещё эта ошибка при запуске pnpm start в Firefox

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

Её надо исправить, иначе я не смогу разрабатывать проект дальше (FF мой основной браузер).

это isue с этой же проблемой https://github.com/vitejs/vite/issues/8621

это вырезка из их документации The worker script can also use import statements instead of importScripts() - note during dev this relies on browser native support and currently only works in Chrome, but for the production build it is compiled away.

единственным способом это починить мне кажется перенос необходиой логики в воркер, тогда если импортировать его не как модуль vite что-то свое импортирует через importScripts который ну вроде не должен ломать ff

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

yorgetsuten commented 1 year ago

Because messages from the worker come after the end of calling runListeners.

i don't think so. reportFreeze only sets the state to a value that does not depend on the time the message came

reportFreeze sets the execution time of each render and at the end sets the value of runListeners execution time. When doing this sequentially, there will always be the sum of the time of all listeners (last call in runListeners). But with a worker, the sequence can change.

fixed it, thanks

yorgetsuten commented 1 year ago

Because reportQuick is no longer called

@sxdav we need to bring it back too of course

im not sure about this solution but as I checked the bug does not appear anymore

ai commented 1 year ago

Судя по багу в ФФ починить module worker (который нужен для Vite) собираются уже в Firefox 111, который релизят на днях. Можем времени забить на эту проблему и дождаться версии 111.

Можешь починить типы:

/home/runner/work/oklch-picker/oklch-picker/lib/paint.ts
  1:15  error  'RenderType' is defined but never used  @typescript-eslint/no-unused-vars

И отдеплдоить текущую версию на свой хостинг, чтобы глянуть как работает продакшен-сборка в ФФ?

yorgetsuten commented 1 year ago

забыл коммит с этим фиксом отправить, сори, а деплоится оно автоматически с каждым коммитом, ссылка та же: https://ephemeral-creponne-3cf2a9.netlify.app/#59.12,0.116,144,100 работает как и раньше, тот же баг с рендером

ai commented 1 year ago

работает как и раньше, тот же баг с рендером

Предлагаю подождать FF 111 и глянуть в нём. Там можно в dev-режиме будет работать и посмотреть по кадрово, что происходит.

ai commented 1 year ago

Нашли другой баг:

  1. Открыть в Хроме https://ephemeral-creponne-3cf2a9.netlify.app/ и https://oklch.com/
  2. Начать двигать ползунок Chroma (не на одно значение, а прямо зажать ползунок и двигать туда-сюда)
  3. Смотреть на график Hue

На старом сайте он меняется без задержкок. На новом с OffscreenCanvas обновляется рывками.

Может проблема в том, что приходит слишком много событий и воркер переполняется?

yorgetsuten commented 1 year ago

Нашли другой баг:

  1. Открыть в Хроме https://ephemeral-creponne-3cf2a9.netlify.app/ и https://oklch.com/
  2. Начать двигать ползунок Chroma (не на одно значение, а прямо зажать ползунок и двигать туда-сюда)
  3. Смотреть на график Hue

На старом сайте он меняется без задержкок. На новом с OffscreenCanvas обновляется рывками.

Может проблема в том, что приходит слишком много событий и воркер переполняется?

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

ai commented 1 year ago

график Chroma рисуется вообще без задержек

Chroma да, а вот Hue нет. Проблема проявляется на Hue (его медленнее рисовать).

yorgetsuten commented 1 year ago

график Chroma рисуется вообще без задержек

Chroma да, а вот Hue нет. Проблема проявляется на Hue (его медленнее рисовать).

ситуация та же только на воркерах и график иногда подлагивает

ai commented 1 year ago

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

Тут надо изучить подходы с очередью заданий.

yorgetsuten commented 1 year ago

очередью заданий.

гуглю

ai commented 1 year ago

Можно сделать такой алгоритм:

  1. Когда посылаешь задание на рендеринг в воркер, то ставишь флаг, что он занет
  2. Если воркер занят, то запоминаешь последнее задание но не присылаешь в воркер
  3. Когда воркер обратно возвращает время рендеринга (главная что закончил), то смотришь есть ли неотправленное задание и отправляешь последнее в воркер.