Poprawa kodu /sfml/ /animacja/

Question

Witam, napisałem proste sterowanie animacjami do gry i chce poświęcić jeden dzień na dopracowanie kodu, aby był wydajny, wygodny i przejrzysty.

Kod ma być użyty w planowanej klasie ruch, która będzie obsługiwana przez klasę sterowanie,
klasa animacja i ruch ma być "silnikiem" (kodem uniwersalnym) a klasa sterowanie ma być rzeczywistym kodem gry który korzysta z "silnika". Aby animacja nie była szarpana przewiduję odgrywanie animacji i nieprzerwanie ruchu, dopóki nie skończy się odgrywać.

Wklajem kod na wklejce aby nie zaśmiecać posta i proszę o komentarz i porady pamiętając o przewidywanym celu tego kodu :)

Animacja.h
http://www.wklej.org/id/3030512/

Animacja.cpp
http://www.wklej.org/id/3030514/

Gracz.h
http://www.wklej.org/id/3030516/

Gracz.cpp
http://www.wklej.org/id/3030518/

Answer 1

Witaj,

Przygotuj się na dużą dawkę konstruktywnej krytyki 

Kod:

• Mieszasz polski z angielskim. Niedobrze.
• Jak na kod, który ma być wielokrotnie wykorzystywany nie napisałeś dokumentacji a nazewnictwo nie pomaga
• Gracz::configureWindow wcale nie konfiguruje okna.

Teraz logika.

• Zacznijmy od założeń. proste sterowanie animacjami do gry Ok! Kod ma być użyty w planowanej klasie ruch i tu jest błąd projektowy. Dlaczego animacja ma być w klasie ruchu? Animacja będąca elementem efektów graficznych ma być elementem klasy związanej z fizyką? Klasa musi spełniać jedno zadanie i mieć jedną odpowiedzialność.
• Taka kalkulacja na szybko. sizeof sf::Texture to 40 bajtów. Każda animacja zawiera teksturę. Każdy gracz dajmy na to 4 animacje. Piszemy skromny system cząsteczkowy na 1000 obiektów. 4*1000*rozmiar_obrazka. Wydajność LEŻY.
• Pomysł polegający na tym, że każda jednostka będzie mieć własny zegar jest co najmniej dziwny. Nie mówiąc o tablicy zegarów. W Supreme Commander było nawet ponad tysiąc jednostek na mapę. Dajmy każdej 2 zegary. 
  Tak wygląda najlepsze założenie:

  void Unit::update(const sf::Time _dt)
  {
  reloadGunAccumulator += _dt;
  animationAccumulator += _dt;
  useBazookaAccumulator += _dt;
  if (reloadGunAccumulator.asSeconds() > 2) {
     this->gun->reload();
     reloadGunAccumulator = sf::Time::Zero;
  }
  if (animationAccumulator >= 0.1) {
     this->updateFrame();
     animationAccumulator = sf::Time::Zero;
  }
  if (useBazookaAccumulator >= 10) {
     this->bazooka->shoot();
     useBazookaAccumulator = sf::Time::Zero;
  }
  }

  Takie zastosowanie pozwala na użycie jednego zegara w całej grze.

• Jedyna produkcja w której postać miała pojęcie o istnieniu okna to Animation vs Animator. Używaj dziedziczenia z sf::Drawable

Comments

const sf::Time& _dt

(to ma tylko lekko kopiowalny long long int, prościej kopiować; jak poprawisz to schowam)

---

To, że każda animacja ma oddzielny czas to efekt tego, że chce aby można było odgrywać animację z różną szybkością

Sizeof sf::Czas = 8, a co do ilości tekstur poprawiłem to trochę, jeszcze nad tym pracuje.
Dziękuję za odpowiedz, za niedługo podeślę wam poprawiony kod do oceny.

Usunąłem klasę gracz i dodałem kilka funkcji do Animacja.
Klasa animacja ma docelowo oddawać odpowiedni sf::Sprite i ma być niezależna od jakiejkolwiek klasy.

Klasa Ruch będzie przyjmować sf::Sprite od klasy animacja i przemieszczać obraz z odpowiednią szybkością na klatkę (tak aby np. noga która odpycha ciało stała w miejscu a nie "ślizgała" się)

Proszę jeszcze o komentarz do tej wypowiedzi

Lekko zmodyfikowany kod:
https://github.com/pajdas/Sfml-project

---

Poczytaj czym jest SOLID bo wszystko co planujesz nie jest z nim zgodne. Klasa move nie powinna mieć nic wspólnego ze spritem. Co najwyżej może mieć metodę wirtualną do przeciążenia w której będziesz przypisywać position do spritea z klasy move.

Masz wybór. Albo mnie się posłuchać i ogarnąć jak to powinno wyglądać, albo zrobić po swojemu i zakopać się w tym projekcie. Kod na pewno nie będzie uniwersalny, a zegar w klasie gracz przy którym się upierasz to implementacyjny absurd.

---

Nie do końca rozumiem, jeżeli

Klasa move nie powinna mieć nic wspólnego ze spritem

To czym ma ta klasa move ruszać, jak nie sprajtem

Albo mnie się posłuchać i ogarnąć jak to powinno wyglądać, albo zrobić po swojemu i zakopać się w tym projekcie

Trochę groźnie to zabrzmiało :) nie planuję Ciebie "niesłuchać" a ten ostatni kod jaki przesłałem to nie wszystko co zamierzam poprawić. Na razie rozwiązuje problem związany z czasem i nie zrobię wszystkiego na raz

---

class Move{
private:

float x,y;

protected:
virtual void assignPosition() = 0;
//...
public:
void updateMove(const sf::Time _dt) {
//...poruszamy
assignPosition();
}
sf::Vector2f getPosition() { return sf::Vector2f(x,y); }
}

Hero : public Move{
private:
sf::Sprite cos;
protected:
virtual void assignPosition() {
cos.setPosition(getPosition());
}
}

Można, Klasa move nic nie wie o istnieniu spirte'a.

---

Ok, dziękuję bardzo, przemyślę jeszcze wszystko

---

Zanim coś, to rzuć okiem na mój projekt 

---

spoko, ja pierwotnie chciałem osiągnąć coś takiego
https://github.com/SFML/SFML/wiki/Source:-AnimatedSprite
ale jak pobrałem sobie to to szybko zauważyłem, że jak naciskam klawisz z dużą częstotliwością, to wygląda to jakby obiekt się ślizgał i dlatego chce zastosować w moim projekcie podział mapy na kafelkowy i odgrywanie animacji do końca, czyli jak zacząłeś to albo się skończy albo zawrócisz :)

---

To zależy od tego czy chcesz animację dziedziczyć czy zawierać.

Answer 2

Hej, w sieci jest sporo przykładów:

https://www.youtube.com/watch?v=Aa8bXSq5LDE

https://www.binpress.com/tutorial/creating-a-city-building-game-with-sfml-part-3-textures-and-animations/125

Dlaczego klasa Gracz zawiera czas i okno? Tym powinna zajmować się pętla główna, a gracz jest osobnym bytem. Ja bym zrobił tak, że Gracz zawiera sprajta, któremu została ustawiona tekstura. Osobna klasa Animacja zawiera zmienna IntRect, który zawiera informację o tym, który fragment sprajta ma być wyświetlany i w pętli głównej gry robię update wszystkiego. Obiekt animacjaGracza dostaje czas z pętli głównej programu, sprawdza czy ma przeskoczyć na kolejną klatkę, a następnie dla obiektu gracz jest ustawiany IntRect (nowy lub stary, zależy czy minął pewien czas). W tym filmiku na yt gościu coś takiego robi, ale on zmienną IntRect traktuje jako publiczną - tak nie rób, stwórz sobie gettera :)