Ocena projektu silnika gier w 3D

Witajcie,

Proszę o ocenę kodu tego "silnika". Na razie w "bennuengine/util/betex" w interfejsie mam std::string itd., ale jak już wyświetlę jakiś model napiszę swój odpowiednik QFile i QString (wtedy Qt tylko dla tych w "zinotengine/tools"), którego będę używał. Na razie tylko trójkąt z VBO, ale pisze teraz taki kod który jest niewidoczny z działania programu. Czyli te tekstury. Komentarze może później na angielski zrobię, chociaż w pisaniu po ang. jestem bardzo średni, albo gorzej.

Aktualne repo: https://github.com/patrykbajos/ZinotEngine
Stare repo (nie uzywane już, stara nazwa): https://bitbucket.org/bajos/bennuengine.git

EDIT: Musiałem zmienić nazwę silnika bo istnieje już projekt co zaczyna się od "Bennu" i jest to jakiś język programowania gier (więcej nie czytałem). http://www.bennugd.org/

PS. Teraz silnikiem tego nazwać nie można, ale cały czas dopisuje coś nowego.

1. Przeciez tu nie ma nic ze silnika
2. uzywanie golego new to antyidom
3. polskie komentarze
4. jezeli musisz komentowac kod to lepiej zrobic z tego funkcje
5. Jezeli klasa ma wszystko statyczne to ja bym po prostu zrobil z tego Same funkcje w jakims namespace i nie robil z tego klasy.

@fasadin, 1. Napisałem o tym, wiem, ale to jest w tworzeniu dopiero. 2. Co w zamian? 3. Wiem, teraz nowy kod staram się po ang. 4. Komentuję każdą prawie linijkę, żeby za ileś tam wiedzieć o co chciałem czynić tym kodem, ale podzielę to... kiedyś tam. 5. Po stworzeniu tematu czytałem jeszcze ten kod i wyszło mi, że Reader, Writer i UTF bo nie ma żadnych atrybutów to static powinny być, albo właśnie w namespace.

1. Tak wiem, ale i tak musialem napisac
2. Poczytaj o RAII
3. to zle robisz. Komentarze powinny byc uzywane jedynie przy opisywaniu skomplikowanego algorytmu (np link czego sie uzywa albo dlaczego tak) lub przy dziwnej optymalizacji. Jezeli musisz komentowac "Pobieranie danych" i piszesz 3 linijki kodu, to zrob z tego funkcje void PobieranieDanych() i juz mozesz wyrzucic komentarze.
   Jezeli chcesz wiedziec wiecej dlaczego komentarze sa zle odsylam do google (albo do ksiazki czysty kod).

5. Static smierdzi jezeli nie wiesz dokladnie jak on dziala. Jest ciezki do debugowania. Lepsze sa po prostu funkcje w namespace (bo sie inicjalizuja przed funkcja main), z nimi nie ma problemu jezeli chodzi o debugowanie

@fasadin, odnośnie pierwszego Twojego posta w czwartym punkcie. Jeśli to matchData podzieliłem na mniejsze metody, które w klasie są prywatne to jeśli wydzieliłbym to do funkcji w namespace zamiast zrobić te wszystkie metody static, jak musiałbym zrobić, aby były nie do użycia poza funkcjami (funkcjami bo już w namespace, a nie static) tej klasy?

Zrobiłem push. Napisałem writer i reader do końca już. Teraz trzeba jakiś test zrobić.

Edit:
Porobiłem małe zmiany bo się nie kompilowało i było parę warningów. Tak btw. to czemu kompilator operacje na void* wykrywa jako piekielne zło i daje warning? Kompiluje się i działa... bo jeszcze nie wykorzystałem tych klas.

Edit2:
Dzisiaj zamierzałem skompilować program na Windows, ale chyba nie uda mi się tego zrobić w czasie. Visual Studio 2013 stroi fochy, twierdzi, że moje konto M$ jest nieaktywne, a jak chcę go usunąć to bez końca robi punkt przywracania systemu.

Spojrzałem tylko na Betex::Reader i to też nie w całości.

1. Zwracanie przez referencję w argumencie nie ma sensu w projektach od początku pisanych w C++11/14. Rób użytek z copy elision i move construction, performance nie ucierpi a czytelność zyska.
2. Powtórzę to co napisał @fasadin, ale bardzo źle korzystasz z możliwości komentowania

// Close file and return success
file.close();
return true;

Nie napisałeś nic, czego nie widać w kodzie.
3. To

std::unique_ptr<uint8_t> rawData;
// ..... //
rawData.reset(new uint8_t[fSize]);

nie będzie działać tak jak się spodziewasz. Jeśli chcesz chronić pamięć zorganizowaną w postać tablicy, musisz poinformować o tym unique_ptr, który ma osobny "deleter" dla tablic.

std::unique_ptr<uint8_t[]> rawData;

4. Gołe wskaźniki w kodzie od początku pisanym w C++11/14 to "big no no".
5. Pozwalasz sobie na zbyt dużą entropię w kodzie, albo jesteś jeszcze na etapie optymistycznego podejścia do kodu. Spójrz przykładowo na funckję:

void Reader::matchHeader(Data & outData)
{
     // Binduje wskaznik na naglowek
    Header * header = (Header *) outData.rawData.get();
    header->toEndian(Endian::Type::UNKNOWN);
    outData.header = header;
}

o policz ile rzeczy może tutaj pójść źle.

(edit)
Nie wiem, czemu mnie naszło na odpowiedź w nieaktywnym wątku ;p

@several, nieaktywnym? Tydzień to chyba nie tak dużo.
Ad4. Gołe wskaźniki to są jeszcze z czasu jak nie wiedziałem o tych *_ptr<*>. Postaram się zastąpić to co istnieje tymi dobrodziejstwami C++11.
Ad5. Wiem o tym, ale to jest tak wszystko pisane na kolanie, żeby działało, nie liczę, że żaden wyjątek nie wywali mi programu (nie cierpię wyjątków). Używanie wyjątków w GE ma sens? Oczekiwanie na wyjątek nie spowolni często wykonywanego kawałka programu? Tutaj chyba to, że gdyby plik był mniejszy niż to 32*4 to się wywali bo nie moja pamięć i nullptr z .get(). Jest jakaś klasa wrapper, że podaje wskaźnik do tablicy i rozmiar i kontroluje czy nie przekraczam zakresu iteratorem albo []?

PS. Ad2. Jakąś mam manię dbania o skomentowany kod nawet bez sensu (boje się, że nie będę wiedział co robi za x czasu) i optymalizacji...

@bajos odnośnie komentarzy, to się z nich raczej nie korzysta. Aby tak komentować kod, teraz raczej pisze się nową metodę i ją odpowiednio nazywa. Wtedy takie komentarze tracą rację bytu. Przykład? Zamiast pisać

// check if basket is empty
if (this.basket.getProducts().count() == 0)
{
   // do something
}

Można napisać

if (this.isBasketEmpty())
{
   // do something
}

Z komentarzami jest problem taki, że się ich z czasem nie aktualizuje przez co tracą na ważności i czasem mogą wprowadzać w błąd. Raczej pisze się je tam, gdzie trzeba wyjaśnić np. dlaczego został użyty akurat taki algorytm, czy przestrzec kolegów przed jakimś zachowaniem kodu. Powodów może być wiele, ale to są raczej pojedyncze przypadki.

@fasadin, widze już zalety tego unique_ptr<>. :D Wcześniej Reader::matchData() było publiczne bo pamięć w Reader::Data na dane z sieci/pliku miałem alokować miejsce poza metodami klasy, a później wywołać odpowiednie metody. Teraz mam pewność, że jak readFromFile zaalokuje pamięć to przy niszczeniu danych zostanie to i tak usunięte bez żadnego sprawdzania czy tam coś jest czy funkcja failowała, albo czy przy kolejnych wywołaniach były już wcześniej jakieś dane.

Ps. Pewnie nie jedyna zaleta :D

Apdejt: Zrobiłem w programie do konwersji tekstur wczytywanie konfiguracji z JSON do klas. Testowałem debuggerem i wartości, które wczytałem zgadzają się z tym co powinno być. Jeden błąd to była tylko pomylona cyferka, przez co internalFormat tekstury się nie zgadzał. Teraz będzie tylko z górki.

PS. To rozwiązanie z QMap enumów we wczytywaniu configów jest dobre, czy jest coś lepszego? Z tego co pamiętam optymalizator switch(...) robi podobne działanie, tylko zamiast jak u mnie enum są wskaźniki do odpowiednich callbacków.

Uwielbiam czytać takie wątki. Każdy kto zamieścił tu kod, został zrównany z ziemią. Ani jeden wątek w tym dziale nie zebrał chyba pochwał.

Cóż, ludzie to tępaki

Mały Kot napisał(a):

Uwielbiam czytać takie wątki. Każdy kto zamieścił tu kod, został zrównany z ziemią. Ani jeden wątek w tym dziale nie zebrał chyba pochwał.

Było całkiem sporo projektów, które zbierały pochwały.

Mały Kot napisał(a):

Cóż, ludzie to tępaki

Prawda. Publikują coś w dziale ocen i wydaje im się, że będą dostawać same gratulacje. Ten dział nie temu służy.

Jednakowoż, nigdy nie widziałem kodu wyprodukowanego przez recenzentów. Hmm ciekawe czy taki piękny jest i czy wszystkie wzorce spełnione wypełnione w 100%

1. Zamiast NULL użyj nullptr
2. nadal brak smart pointerow (np. w engine jest gołe new)
3. Używaj loggera skoro go masz, nie potrzebujesz tego

std::cout << "Hello world!!!" << std::endl; 

do tego znasz różnice pomiedzy std::endl a "\n"? Jestes pewien ze chcesz użyć tutaj endl?
4. Funkcje ogolnie pisze sie z dużej litery (ale jeżeli masz taki styl jaki masz teraz i chcesz się go trzymać to jest ok)
5. createWindow(). Spodziewałbym się że to tworzy okno a nie że obsługuje eventy! (ogólnie popraw nazwy funkcji)
6. Powodzenia z debugowaniem z jednolinijkowcami (chyba, że tak lubisz to zapomnij o tym punkcie)

for (Resource * elem : content) if (elem) delete elem; 

7. Różne formatowanie, raz jedna instrukcja w if jest w tej samej linijce a raz ma {} a raz jest po prostu w nowej linijce bez {}
8. Bezsensowne, nic niewnoszące, po polsku komentarze

@fasadin

Ad1. W kodzie, który piszę teraz już tak robię. Część kody pisana w core była dawno nie ruszana. Jak nałożę już na trójkąt teksturę z mojego formatu to będę przez cały kod się przedzierał i zmieniał te złe rzeczy.
Ad2. Jak wyżej, core było nie ruszane. Smart pointerami zainteresowałem się przez Twój post w tym wątku, a od tego czasu w core się nic nie zmieniało. Na git mam trochę powalone komentarze zmian bo jak zmieniałem nazwy niektórych plików to zmieniło komentarz tam gdzie dany plik był dowiązany.
Ad3. Loggera muszę jeszcze przystosować do wielowątkowości i zrobić, żeby tworzył loga nie w ./ tylko w katalogu usera odpowiednio na Linuxie, Win, Mac.
Ad5. Pisane na szybko ze stronki SFML tylko, żeby wyświetlić ten testowy trójkąt. Wiem, do poprawy, ale jak tylko skończę te tekstury.
Ad6. Dziękuję. :) Ten resMgr i tak pójdzie do wymiany na taki ze smart pointerami.
Ad7. Jak jest tylko jedna linijka w if-ie to nie ma sensu klepać tych nawiasów.
Ad8. Teraz staram się pisać nowy kod bez komentarzy tylko z odpowiednimi nazwami metod itd. Tylko tam gdzie używałem libki ze zrąbanym naming convention użyłem komentarzy. Tak jak w większości przypadków to w starym kodzie jest. Po teksturach celem jest poprawa już istniejącego kodu.

no to slabo widziales bo moje sa dwa w sieci...

No ale kodu wyprodukowanego np przez @somekind nie widziałem nigdzie. A mądrzy się wszędzie, w tym dziale też. Jakie to wzorce są łamane. Ciekawe jaki kod pisze @somekind ktokolwiek widział ktokolwiek wie?

Mały Kot - atakując innych nie pomagasz sobie :)
bronisz się, bo skrytykowali kod i szczerze powiedziawszy słusznie... A wzorców projektowych jest mnóstwo. Poza tym poczytaj o zasadach SOLID oraz KISS czy DRY.

Mały Kot napisał(a):

No ale kodu wyprodukowanego np przez @somekind nie widziałem nigdzie. A mądrzy się wszędzie, w tym dziale też. Jakie to wzorce są łamane. Ciekawe jaki kod pisze @somekind ktokolwiek widział ktokolwiek wie?

Tysiące linii mojego kodu znajdują się w tysiącach postów, które napisałem na tym forum. Jeśli nie byłeś w stanie go znaleźć, a psioczysz pod moim adresem, to znaczy, że masz całkowitą rację w tych słowach:

Mały Kot napisał(a):

Cóż, ludzie to tępaki

Nie odzywałem się w tym wątku, ani też nie oceniałem tu niczego, ale mnie tu przywołałeś... Z czym konkretnie masz problem?

@bajos Projekt/temat umarł?