Modules API #714
Modules API #714
Conversation
Found out that it wasn't really clearing anything. Now it does call clear, but still there are temporal denoising artifacts. The issue remains elusive. Related: w23#661
Also make a way to signal the module whether the shutdown was forced or manual.
| @@ -19,6 +19,15 @@ | |||
|
|
|||
| #include <math.h> // sqrt | |||
|
|
|||
| RVkModule g_module_textures = { | |||
| .name = "textures", | |||
There was a problem hiding this comment.
Тут хорошо бы решить через что мы будем брать имя:
- Через макрос
MODULE_NAME. - Через поле
name.
Мне кажется предпочтительнее 2-ое, потому что этот макрос можно использовать только внутри самого модуля. Если мы, например, захотим в одном модуле узнать имя другого модуля обратившись к нему, то через макрос это не выйдет.
Stuff done during stream E350 - [x] Restore skybox reflection - [x] Improve skybox log messages
Update patches Fix texture coordinates for monitors (c1a0, c1a1f)
Fix traditional render tries to load bluenoise, w23#710
Add new rad files add c2a5a.rad c2a5d.rad c2a5e.rad
Also make a way to signal the module whether the shutdown was forced or manual.
…into vk_modules
Pass name of the module instead of the module itself in print args. Forward declare `cl_entity_s` inside `camera.h`.
| static qboolean Impl_Init( void ) { | ||
| XRVkModule_OnInitStart( g_module_buffer ); | ||
|
|
||
| // Nothing to init for now. | ||
|
|
||
| XRVkModule_OnInitEnd( g_module_buffer ); | ||
| return true; | ||
| } | ||
|
|
||
| static void Impl_Shutdown( void ) { | ||
| XRVkModule_OnShutdownStart( g_module_buffer ); | ||
|
|
||
| // Nothing to clear for now. | ||
|
|
||
| XRVkModule_OnShutdownEnd( g_module_buffer ); | ||
| } |
There was a problem hiding this comment.
Здесь конечно ничего не происходит, но так как я сделал это отдельным модулем, то функции нужно реализовать.
Вынес в модуль потому, что пользуемся функциями из devmem, который должен быть инициализирован. Не хочется делать через костыли, особенно когда мы как раз хотим от них избавиться.
| // TODO(nilsoncore): Integrate all other modules. | ||
| extern RVkModule g_module_textures; | ||
|
|
||
| static const RVkModule *const g_modules[] = { | ||
| &g_module_textures | ||
| }; | ||
|
|
There was a problem hiding this comment.
Убрал, потому что пока не знаю зачем нам хранить один большой список модулей, если мы можем обращаться к ним напрямую, т.к. они все публично предоставляются в заголовках через extern.
| #if USE_AFTERMATH | ||
| if (!VK_AftermathInit()) { | ||
| gEngine.Con_Printf( S_ERROR "Cannot initialize Nvidia Nsight Aftermath SDK\n" ); | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Здесь логику пришлось немного поменять - модуль все равно будет "инициализироваться", даже если макрос не определен. Сделал так, потому что некоторые другие модули имеют этот модуль (nv_aftermath) в зависимостях, а значит они вызовут на него Init(). Какие-то отдельные проверки городить не хочется, т.к. это просто ненужное усложнение.
Если макрос определен - делаем настоящую инициализацию, если нет - то факту ничего не делаем и говорим, что мы инициализировались.
There was a problem hiding this comment.
Думаю, что aftermath это пока "особый случай", наравне с инициализацией вулкан инстанса и устройства. Он может оставаться ровно таким, как был до модулей, можно вообще не трогать пока.
| /* Modules without dependencies */ | ||
|
|
||
| if ( !g_module_combuf.Init() ) return false; | ||
| if ( !g_module_devmem.Init() ) return false; | ||
| if ( !g_module_descriptor.Init() ) return false; | ||
| if ( !g_module_nv_aftermath.Init() ) return false; | ||
|
|
||
| /* Modules with dependencies */ | ||
|
|
||
| if ( !g_module_buffer.Init() ) return false; | ||
| if ( !g_module_staging.Init() ) return false; | ||
| if ( !g_module_image.Init() ) return false; | ||
| if ( !g_module_pipeline.Init() ) return false; | ||
| if ( !g_module_framectl.Init() ) return false; | ||
| if ( !g_module_geometry.Init() ) return false; | ||
| if ( !g_module_render.Init() ) return false; | ||
| if ( !g_module_studio.Init() ) return false; | ||
| if ( !g_module_scene.Init() ) return false; | ||
| if ( !g_module_textures_api.Init() ) return false; // +g_module_textures (hardcoded) -- @TexturesInitHardcode | ||
| if ( !g_module_overlay.Init() ) return false; | ||
| if ( !g_module_brush.Init() ) return false; |
There was a problem hiding this comment.
Оставил пока что инициализацию столбиком как было раньше. Пока что не определился - делать ли vk_core как модуль и прописывать ему все нужные зависимости, которые он будет инициализировать, или просто оставить как внешний код, который просто сам инициализирует нужные модули.
По идее хорошо бы его тоже модулем сделать, чтобы вот это вот не городить, а он просто вызвал инициализацию зависимостей и все работало по-человечески, через модульное API.
Но есть загвоздка - все модули ожидают, что vk_core уже инициализирован. Для этого ему нужно самому сделать все необходимые действия - создать инстанс, выбрать физ. девайс, создать логический девайс и т.д. - всё это должно произойти до того, как мы будем вызывать любой из модулей (по-хорошему). Но если вот это "ядро" сделать модулем, то получается, что он сначала пойдет инициализировать зависимости перед тем, как инициализироваться самому.
Хотя! Я сейчас подумал и вроде как есть решение - внутри Impl_Init() можно сначала сделать всю инициализацию, и только потом "вызывать" XRVkModule_OnInitStart(). Это конечно ломает стандартную логику модуля, где мы должны это писать как-раз таки до инициализации, но здесь можно сделать исключение, т.к. это можно сказать "корневой" модуль, от которого будут подтягиваться остальные.
There was a problem hiding this comment.
Пока не нужно. На текущем этапе в модули надо сконвертить только большую часть того, для чего тут зовутся Init/Shutdown. Что конвертится с трудом, или плохо/нетривиально мапится на модули, можно не трогать.
Разбиение на более атомарные модули может прагматично понадобиться (или не понадобиться!) позже, с опытом работы с модулями. С той горы виднее будет, но пока мы на неё не взошли.
| if ( vk_core.rtx ) { | ||
| g_module_light.Shutdown(); | ||
| g_module_rtx.Shutdown(); | ||
| } |
There was a problem hiding this comment.
Проверка на vk_core.rtx лишняя, т.к. мы в любом случае "инициализируем" модуль, пусть и не по-настоящему - та же механика, что и с nv_aftermath, только вместо макроса тут обычная проверка if-ом.
| @@ -18,8 +33,6 @@ static struct { | |||
| int texture_index; | |||
|
|
|||
| vk_render_type_e render_type; | |||
|
|
|||
| qboolean initialized; | |||
There was a problem hiding this comment.
initialized больше не нужен, т.к. состояние инициализации уже хранится внутри модуля.
| // NOTE(nilsoncore): Moved from `vk_beams.c`. | ||
| // It uses global `g_camera` and comment suggests to use `R_SetupFrustum` -- a function inside `camera.c` -- so probably it is a good place. | ||
| #define RP_NORMALPASS() true // FIXME ??? | ||
| int CL_FxBlend( cl_entity_t *e ) // FIXME do R_SetupFrustum: , vec3_t vforward ) |
There was a problem hiding this comment.
Перемещено из scene для избежания циклической зависимости, подробнее в scene.
There was a problem hiding this comment.
Камера для этой функции тоже не лучшее место. Если уж хочется перенести, то лучше в какой-нибудь r_common, или там r_misc, или что-нибудь похожее, что у нас там есть или может быть.
| @@ -986,3 +923,105 @@ void R_TextureRelease( unsigned int texnum ) { | |||
| const qboolean ref_interface = false; | |||
| releaseTexture( texnum, ref_interface ); | |||
| } | |||
|
|
|||
| static qboolean Impl_Init( void ) { | |||
There was a problem hiding this comment.
Заранее извиняюсь за множество затронутых +- линий, которые на самом деле практически те же, просто все предыдущие функции Init и Shutdown перенесены в конец файла, т.к. до этого они постоянно были в разных местах.
There was a problem hiding this comment.
Пускай будут в разных местах, красоту можно по-желанию навести потом отдельными коммитами, ревью которых будет делать тривиально.
| // NOTE(nilsoncore): This has to be hardcoded in a current approach. -- @TexturesInitHardcode | ||
| // They use each other's functions, so it will be a circular dependency if we include them in a normal way. | ||
| // We could instead not turn `textures` (vk_textures{h,c}) into a whole module and leave it as it is, | ||
| // but it has `Init` and `Shutdown` functions like in a normal module, so it's bad either way. | ||
| // This probably needs some reorganization / change of logic. | ||
| g_module_textures.init_caller = &g_module_textures_api; | ||
| if ( !g_module_textures.Init() ) { | ||
| ERR( "Couldn't initialize '%s' submodule.", g_module_textures.name ); | ||
| g_module_textures_api.state = RVkModuleState_NotInitialized; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
vk_textures - это вроде бы просто внутренности r_textures, но у них есть свои Init и Shutdown, странно просто оставлять не тронутым. И, кстати, кто-то использует именно vk_textures без r_speeds, что странно. Не знаю, не хочется вот так оставлять файлы "в темноте", когда у них есть явный интерфейс инитов/деинитов, у них есть зависимости от других модулей и т.д. С другой стороны, будут возникать вот такие костыли. Наверно по итогу стоит реорганизовать компоненты так, чтобы такого не происходило в принципе.
There was a problem hiding this comment.
На текущий момент r_textures и vk_textures можно считать одним модулем. По идее это должно быть сильно проще.
| g_module_brush.Shutdown(); | ||
| g_module_overlay.Shutdown(); | ||
| g_module_textures_api.Shutdown(); | ||
| g_module_textures.Shutdown(); | ||
| g_module_scene.Shutdown(); | ||
| g_module_studio.Shutdown(); | ||
| g_module_render.Shutdown(); | ||
| g_module_geometry.Shutdown(); | ||
| g_module_framectl.Shutdown(); | ||
| g_module_pipeline.Shutdown(); | ||
| g_module_staging.Shutdown(); | ||
| g_module_buffer.Shutdown(); | ||
|
|
||
| g_module_nv_aftermath.Shutdown(); | ||
| g_module_descriptor.Shutdown(); | ||
| g_module_devmem.Shutdown(); | ||
| g_module_combuf.Shutdown(); |
There was a problem hiding this comment.
Вот этого безобразия, конечно же, не должно быть - тут все то же самое, что и в предыдущем комментарии.
Однако тут дополнительный вопрос - должен ли модуль при своем Shutdown вызывать Shutdown зависимых модулей? Или модули сами должны выключаться по достижению reference_count == 0? Трекинг референсов я еще, кстати, не сделал.
Я пока не уверен как мы должны считать референсы (которые, насколько я понимаю, хранят количество внешних активных модулей, которые зависят от данного модуля?).
There was a problem hiding this comment.
В простом текущем варианте модуль должен делать "Acquire" своим зависимостями (refcount++ и инициализацию, если refcount был 0) при инициализации, и "Release", когда его гасят.
"Release" внутри себя уменьшает refcount, и делает настоящий Shutdown только при достижении им нуля.
Тут напрашивается где-то в самом конце R_VkShutdown() проверка на то, что все-все модули были погашены. Тут можно либо завести глобальный счётчик количества инициализированных модулей, либо глобальный список модулей. Счётчик проще сделать, и он будет железобетонный, но тогда надо будет грепать логи на предмет поиска непогашенного. В список можно будет забыть добавить и не обнаружить утечку.
|
Выкатил первую рабочую версию, НО - проверял только в растере, в RT рендере скорее всего не работает. Возможно оно будет работать, если происходит запуск сразу на RT, но переключение между рендерами точно работать не должно из-за логики работы связанных с RT модулями - подробнее в комментариях ревью. Пока что все сообщения инитов / деинитов пишутся в консоль, но скорее всего нужно будет добавить что-то, что будет это контролировать, т.к. спамит достаточно. Хотя сразу видно какой модуль когда включен, когда выключен. Как это реализовать пока что не знаю - сделать ручную проверку аргумента типа Если честно мне уже тошно от этого и пишу все это через силу, но я понимаю, что никто за меня ход моих мыслей и идей объяснять не будет, поэтому приходится. По сути большая часть работы сделана, осталось довести до конца, отполировать к финальному, готовому виду. К сожалению коммиты читать очень тяжело, потому что они в спаме изменений из-за того, что я все функции инитов и деинитов перенес в конец файлов, т.к. они в разных файлах были в разных местах, плюс если они в конце файла, то никакие лишние форвард-декларации плодить не придется. Во всяком случае постарался писать комментарии в ревью под значимыми кусками кода, чтобы не потеряться в спаме. На этом, наверно, пока все - буду ждать разбора на стриме. А до этого, я наверно немного развеюсь, потому что реально уже голова кипит мало того, что от комплексности самой проблемы и методов реализаций, так еще и от того, что это нужно все объяснять... В общем, надеюсь меня поняли. |
|
Мержить пока нельзя, есть еще косяки - просто жду ревью текущих изменений. |
|
Т.к. во вкладке измененных файлов будет много лишних коммитов, в том числе не моих из-за
Для этого нужно зайти во вкладку коммитов и пролистать в самый низ (новые коммиты начинаются снизу).
|
|
Это гигантская работа, спасибо! |
There was a problem hiding this comment.
Так, я очень устал и сделал только %40 ревью.
Это очень большая работа, очень спасибо!
Пока что самые главные замечания это:
- Не надо впредь делать rebase. Этот очень грубый инструмент сносит башню всему на свете. Rebase можно делать только крайне аккуратно на личных work-in-progress ветках, которые никому не показываются, никуда не пушатся, и тем более не участвуют в PR. Нам скорее всего придётся в итоге закрыть этот PR, создать новую ветку через ручное протаскивание изменений, и открыть новый PR.
- Не надо переносить функции Init/Shutdown и трогать их внутренности. Это раздувает PR и делает его нечитаемым. Достаточно просто добавить RVkModule структуру в самый конец файла, ссылаясь на существующие имена функций.
- Не надо переименовывать функции. Не уникальные имена функций значительно усложнают грепанье кода.
- Не надо разбивать на мелкие модули. Не каждый файл или сущность модуль. Модули это более-менее то, что имеет Init/Shutdown пару в vk_core, с единичными исключениями.
There was a problem hiding this comment.
Здесь и далее гитхабик желает замержить файлы старыми версиями. Скорее всего это из-за ребейза.
Ребейз очень грубый инструмент, он явно и неявно ломает кучу всего. Его не рекомендуется использовать за исключением особых случаев, и очень хорошо подумав о последствиях.
Скорее всего из-за него этот PR окажется невосстановим, и придётся для мержа открывать новый, потеряв/разделив часть переписки.
| // NOTE(nilsoncore): Moved from `vk_beams.c`. | ||
| // It uses global `g_camera` and comment suggests to use `R_SetupFrustum` -- a function inside `camera.c` -- so probably it is a good place. | ||
| #define RP_NORMALPASS() true // FIXME ??? | ||
| int CL_FxBlend( cl_entity_t *e ) // FIXME do R_SetupFrustum: , vec3_t vforward ) |
There was a problem hiding this comment.
Камера для этой функции тоже не лучшее место. Если уж хочется перенести, то лучше в какой-нибудь r_common, или там r_misc, или что-нибудь похожее, что у нас там есть или может быть.
| @@ -22,6 +22,30 @@ | |||
| #define MODULE_NAME "textures" | |||
| #define LOG_MODULE tex | |||
|
|
|||
| static qboolean Impl_Init( void ); | |||
There was a problem hiding this comment.
Неудачное общее имя -- очень сложно будет грепать по функциям, когда в куче файлов они одинаковые.
Лучше в большинстве случаев вернуть оригинальные имена.
| @@ -42,93 +66,6 @@ static void destroyTexture( uint texnum ); | |||
|
|
|||
| #define R_TextureUploadFromBufferNew(name, pic, flags) R_TextureUploadFromBuffer(name, pic, flags, /*update_only=*/false) | |||
|
|
|||
| qboolean R_TexturesInit( void ) { | |||
There was a problem hiding this comment.
Не вижу смысла в переносе функций. Это раздувает дифф и делает ревью более трудоёмким (потенциально гораздо более сложным и почти нечитаемым, если диффовый распознаватель споткнётся и начнёт пытаться мержить с какими-нибудь другими функциями, он так иногда умеет).
| static qboolean skyboxLoad(const_string_view_t base, const char *prefix) { | ||
| static qboolean skyboxLoadF(const char *fmt, ...) { |
| @@ -11,6 +13,8 @@ | |||
|
|
|||
| #include "shaders/ray_interop.h" | |||
|
|
|||
| extern RVkModule g_module_ray_model; | |||
There was a problem hiding this comment.
Тут по идее нужно только динамический g_module_rt_dynamic_model сделать модулем.
| // R_RenderModelCreate: RT_ModelCreate | ||
| // R_RenderModelDestroy: RT_ModelDestroy | ||
| // R_RenderModelUpdate: RT_ModelUpdate | ||
| // R_RenderModelUpdateMaterials: RT_ModelUpdateMaterials | ||
| // R_RenderModelDraw: RT_FrameAddModel | ||
| // R_RenderDrawOnce: RT_FrameAddOnce | ||
| // &g_module_ray_model, -- NOTE(nilsoncore): For some reason we don't see this module although we use its functions?? Whatever, it's inside rtx anyway. |
There was a problem hiding this comment.
Да, в лучах тут некоторый беспорядок, который рано или поздно напрашивается на распутывание.
| // FIXME where should this function be | ||
| #define RP_NORMALPASS() true // FIXME ??? | ||
| int CL_FxBlend( cl_entity_t *e ) // FIXME do R_SetupFrustum: , vec3_t vforward ) |
There was a problem hiding this comment.
Бтв, это stateless функция, для неё не надо ничего инициализировать. Так что можно и на месте оставить.
| // R_DrawSpriteQuad: R_VkMaterialModeFromRenderType -- @MaterialMode: Why is it in ray_model and not in render? | ||
| &g_module_ray_model, |
There was a problem hiding this comment.
R_VkMaterialModeFromRenderType() это stateless вспомогательная функция, для её использования не нужен модуль.
|
|
||
| XRVkModule_OnInitEnd( g_module_sprite ); | ||
| return true; | ||
| // TODO return createQuadModel(); |
There was a problem hiding this comment.
Там какая-то шляпа была с порядком инициализации, пусть пока будет, как есть.
|
Йоу @nilsoncore чё каво? |
|
Йоу @w23 Решил я таки добить этот PR, но в текущем виде скорее всего это уже невозможно - услышал где-то про элегантность ребейза и зафакапил тут все. Скорее всего придется создать новый и ссылаться на этот для истории обсуждения. Если честно, придется заново пробежаться по всему, что я тут наплодил и вникнуть, благо комментарии вроде обширные оставлял. Комментарии все прочитал и учту в новом PR, постараюсь не раздувать лишним.
Как сказал выше, думаю все же допилить, но если сильно надо и есть прям четкое видение как все это реализовать, то конечно, я не против, могу заняться чем-то другим, потому что пока насчет финального вида я на 100% не уверен, плюс забыл все, надо вспоминать. |
Ага, мы тут все выпали и потеряли контекст.
Я всё ещё не смотрел на это внимательно, и абсолютно чёткого видения нет. Но емнип я тут в комментариях ревью накидывал примерный вектор движения, и там начинали вырисовываться очертания желаемого. |
|
Так, пролистал комментарии, и ничего там не вырисовывается. Вернее, оно вырисовывается, но только у меня в голове, никуда напоказ я это не вывалил. Предлагаю попробовать пройтись тут в комментах через лёгкое дизайн-ревью, т.е. прежде чем писать какой-то код, примерно накидать требования и обсудить апи. И только потом восстанавливать подходящие куски кода из этой ветки в свежую. Например, сыро и тезисами, что хотим:Зависимости
Времена жизни модулей
Валидация (опционально)
АппендиксТут, кстати, можно озадачиться тем, чтобы перевести модули со статической модели (где у них есть Почему:
Как делать
|
Как мы знаем, все наши
Vulkan-овские модули внутриref/vkтак или иначе имеют концепцию инициализации и деинициализации посредством вызова функций:xxx_Init()- для инициализации модуля перед тем, как его использовать.xxx_Shutdown()- для деинициализации модуля после того, как им закончили пользоваться.Данная концепция подразумевает, что перед тем, как использовать что-то из нужного нам модуля, мы сначала должны этот модуль инициализировать, а когда он нам не нужен, то мы вызываем деинициализацию, чтобы этот модуль сделал все необходимые действия для плавного возвращения всех ресурсов, которые он занял, обратно в систему.
Реализовано это у нас очень просто, но эта простота упускает реализацию достаточно важных деталей:
Состояние инициализации
Мы его просто напросто не знаем. То есть каждый раз перед тем, как использовать функционал модуля, мы должны либо инициализировать его, даже если он уже инициализирован, так как мы не знаем об этом, либо надеяться на то, что кто-то уже сделал это до нас.
Зависимости
Все они задаются вручную и никак не отслеживаются самим модулем. Нет какого-то определенного списка зависимостей, который мы просто можем взять и посмотреть, а также, чтобы это сделал сам модуль и проинициализировал их, если это еще не было сделано. "Списки" этих зависимостей нигде не хранятся, кроме как в самом коде в виде вызовов их функций инициализации, которые обычно идут списком внутри функции
xxx_Init().Этот PR является попыткой все это реорганизовать и вынести в отдельный, доведенный до ума API. Намеки на надобность такого рода API уже есть комментариях самого кода:
xash3d-fwgs/ref/vk/vk_core.c
Lines 680 to 707 in a0b36a4
Осталось лишь понять как именно этот API реализовать и что конкретно мы от него хотим. Я выделил следующие критерии (за исключением всяких "простота в использовании", "понятность" и т.д. - берем это, как базовые критерии):
Init-ы внутри другихInit-ов.Initи были уверены в том, что все зависимости будут прогружены и модуль будет готов к работе.Это был небольшой
Specification, надеюсь мои мысли и идеи совпадают с первоначальными.Вот как примерно выглядит код инициализации и деинициализации модулей сейчас:
xash3d-fwgs/ref/vk/vk_core.c
Lines 765 to 783 in a0b36a4
xash3d-fwgs/ref/vk/vk_core.c
Lines 838 to 853 in a0b36a4
Первоначально хотел показать реализацию непосредственно внедрив ее сразу, но столкнулся с тем, что не все модули имеют простой
Initбез аргументов - некоторые ожидают параметры, которые создаются на этапе других модулей, которые зависят от других и так далее...Такая проблема есть в
vk_swapchain, где инициализация ожидает внешние параметры:xash3d-fwgs/ref/vk/vk_swapchain.c
Line 184 in a0b36a4
Эти параметры потом сохраняются в глобальное состояние:
xash3d-fwgs/ref/vk/vk_swapchain.c
Lines 196 to 197 in a0b36a4
Если посмотреть саму структуру где они хранятся, то видим комментарий, что они тут быть и не должны:
xash3d-fwgs/ref/vk/vk_swapchain.c
Lines 9 to 12 in a0b36a4
Сам модуль
vk_swapchainинициализируется в модулеvk_framectl(кстати не знаю что заframectl, понятно лишь то, чтоframe- значит связано с кадрами, #709):xash3d-fwgs/ref/vk/vk_framectl.c
Lines 420 to 432 in a0b36a4
Идем в заголовок и видим, что и здесь это не должно быть:
xash3d-fwgs/ref/vk/vk_framectl.h
Lines 13 to 22 in a0b36a4
Если посмотреть чуть выше, то видим комментарий, что тут вообще всех этих глобальных переменных не должно быть:
https://github.com/w23/xash3d-fwgs/blob/a0b36a4301fbd718a4c3ae85bbe06d9efa50d6df/ref/vk/vk_framectl.h#L8C1-L8C1
// TODO most of the things below should not be global. Instead, they should be passed as an argument/context to all the drawing functions that want this infoТакой вложенности мне хватило, чтобы понять, что сам я тут точно не разберусь...
Наверно это надо в отдельную ишью вынести, ну ладно, написал значит написал, может потом создам, где сразу для всех модулей выпишу эти зависимости.
Жду отзывов на данное творение и возможные предложения/исправления.