maszyna/pr-reviews/PR-224.md

Review PR #224 — Migrate from float3d to glm (second approach)

Autor: @docentYT · base: master · 22 pliki, +204/−639 https://github.com/MaSzyna-EU07/maszyna/pull/224

Werdykt: Request changes — build VR, opis, kilka miejsc do sprawdzenia

Podsumowanie

Usunięcie utilities/Float3d.{h,cpp} (float3, float4, float4x4) na rzecz glm::vec3/quat/mat4. TSubModel::fMatrix z wskaźnika (w unii z iMatrix) staje się wartością glm::mat4 {1.0f}; o tym, czy macierz jest niejednostkowa, decyduje już tylko flaga 0x8000. Serializacja .e3d przez nowe utilities/serialize.h.

Co sprawdziłem (OK)

• Kolejność elementów macierzy w .e3d/.t3d: float4x4::operator()(x)[y] = e[4x+y], a glm::make_mat4(e) jest column-major, więc matrix[x][y] w nowym readMatrix i w (de)serialize_float32/64 daje dokładnie ten sam układ bajtów. ✔ Pliki binarne pozostają zgodne w obie strony.
• InitialRotateMatrix() jest wierną kopią float4x4::InitialRotate(). ✔
• Brak new/delete dla macierzy — uwagi z poprzedniej rundy (@jakubg1, @Hirek193) zaadresowane. ✔
• BinInit: m.size() > iMatrix przy iMatrix == -1 — -1 konwertuje się do size_t max, więc warunek jest fałszywy i zostaje jednostkowa. Działa, ale patrz pkt 4.

Uwagi blokujące

1. Build z OpenVR się nie skompiluje. vr/openvr_imp.cpp (linie ~253 i ~349 na master) woła TSubModel::ReplaceMatrix(), które ten PR usuwa, a pliku vr/ PR nie zmienia. CI buduje bez VR, więc tego nie złapie. Trzeba podmienić na przypisanie fMatrix i ustawienie iFlags |= 0x8000 (patrz pkt 2).
2. Flaga 0x8000 jest teraz jedynym źródłem prawdy. Wcześniej renderer mnożył przez macierz, gdy wskaźnik nie był pusty. Teraz każde miejsce, które przypisuje fMatrix z zewnątrz (VR, ewentualne skrypty, przyszły kod), musi też ustawić flagę, inaczej macierz zostanie po cichu zignorowana. Proponuję setter SetMatrix(glm::mat4 const&), który ustawia/kasuje flagę wg == glm::mat4(1.0f), i prywatne fMatrix.
3. Inne gałęzie i PR-y. Usunięcie Float3d.h łamie każdy otwarty kod, który używa float3 — np. #261 dodaje nowe SetRotate(float3(1, 0, 0), …), a feat/gl46-migration (#216) pewnie też go używa. Proszę ustalić kolejność merge z maintainerami i zostawić komentarz w #261/#216. Sprawdzić też betterRenderer/ (np. IntelTAA.h ma trafienia float3; to prawdopodobnie HLSL, ale build z WITH_BETTER_RENDERER=ON trzeba przepuścić).

Pozostałe uwagi

4. if (m.size() > iMatrix) — porównanie size_t z int (GCC -Wsign-compare, Sonar). Lepiej if (iMatrix >= 0 && static_cast<std::size_t>(iMatrix) < m.size()). To też jawnie pokazuje intencję. Przy okazji: indeks poza zakresem w uszkodzonym pliku był wcześniej UB, teraz jest cicho ignorowany — warto zalogować Bad model.
5. Zakomentowany kod w ParentMatrix() z dopiskiem „It will not work, fMatrix is not a pointer anymore” — lepiej usunąć cały martwy blok (Sonar S125).
6. TSubModel *Root … TODO: Fix missing delete — jeśli to znany wyciek, lepiej osobne issue niż TODO w kodzie wprowadzone przy okazji.
7. Pamięć: każdy TSubModel ma teraz 64 B macierzy zamiast 8 B wskaźnika, a submodele binarne nie współdzielą już tablicy Matrices. Dla dużych scenerii (tysiące instancji modeli) warto zmierzyć zużycie RAM przed/po — opis wymienia to jako „potential downside”, przydałaby się liczba.
8. Opis PR jest bardzo skąpy jak na zmianę tej skali. Proszę dopisać: co testowano (np. wczytanie kilku scenerii z .t3d i .e3d, konwersja convertmodels, porównanie zrzutów ekranu, pantografy — tam jest dużo matematyki macierzowej), oraz że format plików się nie zmienia.
9. Formatowanie: fMatrix =glm::mat4(); (brak spacji), mieszane wcięcia w DynObj.cpp (spacje vs tabulatory w zmienianych liniach).
10. SafeNormalize w glmHelpers.h — brak nowej linii na końcu pliku.