• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Prośba o code-review

0 głosów
250 wizyt
pytanie zadane 14 lutego 2020 w Nasze projekty przez obl Maniak (51,120 p.)
edycja 14 lutego 2020 przez obl

Witam, stworzyłem jakiś czas temu dwa większe projekty aplikacji w Angularze 8 z niedawnym updatem do Angulara 9.

Pierwszy projekt to gra umożliwiająca tworzenie własnej mapy, zapis / odczyt z pliku oraz uruchomienie w trybie gracza. Kod do gry umieściłem w repozytorium https://github.com/Obliczeniowo/DeadlyCannons

Grę można zobaczyć w akcji na mojej stronie internetowej: https://www.obliczeniowo.com.pl/1147

Drugi projekt to program do obliczania kratownic statycznie niewyznaczalnych: https://github.com/Obliczeniowo/TrussCalculator

Pogram w działającej wersji jest dostępny na mojej stronie https://www.obliczeniowo.com.pl/1152

Chciałbym wiedzieć co poprawić, ulepszyć jakie błędy popełniam.

1
komentarz 14 lutego 2020 przez Chess Szeryf (76,750 p.)
edycja 14 lutego 2020 przez Chess

game bug maybe

Znalazłem chyba buga w Twojej pracy. Wszedłem w edytor map, zacząłęm tworzyć nową mapę, po czym wyszłem z edytora coś tam jeszcze może ponaciskałem, zostawiłem tamtą mapę i zacząłem tworzyć jeszcze jedną nową. Nacisnąłem tam na domyślne powiększ mapę na chyba 50x50, po czym nacisnąłem "play". W grze nie został wyrenderowany obiekt, jakim jest zielony kolor bloku jak widać na załączonym obrazku, jest czarny.

Nie wiem jak renderujesz przemieszczanie się pocisków oraz czołgu, ale możesz to zoptymalizować chyba jakoś, np. jeśli gracz przyciska klawisz myszy/klawiatury, to czołg niech nie jedzie co jeden np. piksel, tylko np. o 20 pikseli. Płynność poruszania się czołgu po takiej zmianie mógłbyś chyba uzyskać pisząc animację do czołgu. Jeśli ktoś przytrzymuje klawisz np. na 2 sekundy, to wtedy to czołg mógłby przez 2 sekundy do wyznaczonego miejsca być w animacji, czyli wyglądałoby tak jakby gracz nim sterował, a w rzeczywistości byłby tylko animacją. Myślę, że to odciążyłoby renderer, jakbyś o tym pomyślał, ale nie wiem, czy to prawidłowe podejście do ewentualnej optymalizacji. Z pociskami mógłbyś zobaczyć i zakodować anologicznie do czołgu.

movement

Nie wiem jak jest w JavaScript, ale chyba, gdy ktoś naciska na klawisz i jest keydown, to tak jakby była pętla nieskończona while(true) i dopóki gracz nie "puści" klawisza, to cały kod, który jest umieszczony w funkcji, która jest wywoływana z tego zdarzenia jest wykonywany.

Gdy napiszesz to na animacji, to gdy jest keydown uruchamiasz zdarzenie, kod się wykonuje w środku, powinien być tam chyba czasomierz, zmienna która będzie ustawiona na 0 i "odpinasz" event listener, czyli, że gdy przytrzymujesz klawisz, to event listener nie powinien być uruchamiany dopóki nie będzie np. flagi ustawionej na 1, która poinformuje keydown, że keyup został uruchomiony i że keydown znów może zacząć działać od "nowa", czyli zareagować na zdarzenie keydown.

Moim zdaniem, jeśli to z animacją jest trafnym pomysłem, to wtedy to jest najprawdopodobniej lepsze, ponieważ przesuwanie np. czołgu ciągle o 1 piksel jest real-time, czyli, że za każdym razem silnik gry musi to obliczać i musi to być tak jakby na (heap - sterta) o ile nie jest. Można to np. porównać do tablic zwykłych (int arr[2];) i dynamicznych (... = malloc(8);).

Natomiast na animacji raz, gdzieś silnik policzy/załaduje animację i będzie włączał w odpowiednim momencie.

komentarz 14 lutego 2020 przez obl Maniak (51,120 p.)
Już naprawiłem dzięki. Co do drugiego zarzutu to nie rozumiem. Mam przemieszczać skokowo? Poza tym  czołg ma wektor przemieszczenia i zadanie ruchu do punktu końcowego krokowo.po ścieżce, którą wyznaczył algorytm znajdujący najkrótszą drogę pomiędzy punktami
komentarz 14 lutego 2020 przez obl Maniak (51,120 p.)
edycja 14 lutego 2020 przez obl

@Chess,
 Gdybym robił animację tak jak mówisz to pociski by się poruszały tylko wtedy, gdy czołg się przemieszcza. Cała mapa na canvasie się musi odrysować. No może nie cała ale ta część, która jest w obszarze rysowania. Do animacji dochodzi to, że wieżyczka czołgu zmienia położenie w relacji z kursorem myszki.

komentarz 14 lutego 2020 przez Chess Szeryf (76,750 p.)
edycja 14 lutego 2020 przez Chess

Jeśli masz np. współrzędne 0, 0 i czołg ma iść/jechać do 30, 0, to zamiast liczyć i stawiać czołg co 1 piksel w danym miejscu, piszesz jakąś animację i czołg sam jedzie do punktu 30, 0.

Nie wiem, czy ten zamysł jest uzasadniony i pożądany w takiej grze, ale mogłoby to wyglądać np. tak.

Możesz napisać skrypt, który wykoknuje animacje i obsługuje przezroczystość. Np.

blocks = new("transparent", x : 30, y : 0, { return (x : 30, y : 0) });
blocks.tank_position = (x : 0, y : 0);
*** ... ***
delete blocks;

blocks = new("transparent", x : 30, y : 8, { return (x : 30, y : 8) });
blocks.tank_position = (x : 30, y : 0);

*** ... ***

Piszesz po prostu taki skrypt graficzny, który wykona animację np. najpierw 30 bloków poziomo przezroczystych i czołg wie, że ma jechać poziomo po tym przezroczystym płótnie o 30 pikseli np. w prawo i tego nie liczysz po prostu przekazujesz dane do kodu odpowiedzialnego za animację. Kod ten narysuje 30 bloków przezroczystych i położy na mapę z grą. Następnie np. czołg będzie jechał po 30. blokach przezroczystych np. z prędkością 3. ms (mili-sekund). Na samym końcu, gdy czołg dotrze na miejsce, wymazujesz, wykonujesz delete tego przezroczystego płótna i tyle. Dane z algorytmu, który oblicza najkrótszą drogę umieszczasz w kodzie "skryptu graficznego". Czyli wrzucić tam mógłbyś kierunek, ile ma narysować bloków, z jaką szybkością ma poruszać się czołg lub wpisać wartość domyślną, sposobów jest wiele.

komentarz 14 lutego 2020 przez obl Maniak (51,120 p.)
Rozumiem twój zamysł, ale musiałbym wyliczać obszar odświeżania i tylko ten obszar odrysowywać kompletnie (z tłem, przemieszczającym się np w kierunku czołgu pociskiem). Dodatkowo nie uwzględniasz faktu, że cały czas wieżyczka czołgu się obraca podążając za kursorem myszy.
1
komentarz 14 lutego 2020 przez Chess Szeryf (76,750 p.)
edycja 14 lutego 2020 przez Chess

Akurat wieżyczkę mógłbyś zaprogramować tak, żeby była skoordynowana z animacją, czyli jeśli czołg przemieszczałby się np. o 3 ms, to wieżyczka także przemieszczałaby na tych samych współrzędnych co czołg odjąć, dodać offset i dodatkowo podążałaby za kursorem myszy.

Co do pierwszego zdania, chyba nie zrozumiałeś jak miałaby wyglądać ta animacja. Chodzi mi o klatki animacji, coś jak w gimp, masz 1. klatkę, 2., 3., itd. Jeśli czołg jechałby o 30 bloków w prawo, to np. blok ma 12 pikseli szerokości i wysokości, więc 12*30 = 360 klatek animacji dla poruszania się czołgu, co jeden piksel. Ale to byłoby już z góry znane z "innego skryptu - graficznego", a nie z tego, gdzie jest zaprogramowana gra, tak jakbyś z programu graficznego ściągał zanimowane grafiki i wrzucał do gry.

<body style="background: gray; margin: 0; padding: 0;">

<pre>
[(==)|    ]
[    |(==)]
</pre>
<div id="tank" style="background: green; position: absolute; width: 50px; height: 50px; left: 0px; top: 0px;"></div>
class Animation_Movement_Tank {
  constructor(width, height, direction, tank) {
    this.width = width;
    this.height = height;
    this.direction = direction;
   
    this.tank = tank;
  }
  
  animate_frames(offset) {
    switch (this.direction) {
      case 0: {
        let i = this.width - 1;
        var clear_interval = setInterval(function() {
      
          this.tank.style.left = offset + i + "px";
          i--;
          if(i <= 0) {
            clearInterval(clear_interval);
          }
      
        }, 50);
        break;
      }
      case 1: {
        let i = 0;
        var clear_interval = setInterval(() => {
          
          this.tank.style.left = offset + i + "px";
          i++;
          
          if(i >= this.width - 1) {
            clearInterval(clear_interval);
          }
          
        }, 50);
        break;
      }
      case 2: {
        let i = this.height - 1;
        var clear_interval = setInterval(function() {
          
          this.tank.style.top = offset + i + "px";
          i--;
          if(i <= 0) {
            clearInterval(clear_interval);
          }
          
        }, 50);
        break;
      }
      case 3: {
        let i = 0;
        var clear_interval = setInterval(() => {
          
          this.tank.style.top = offset + i + "px";
          i++;
          
          if(i >= this.height - 1) {
            clearInterval(clear_interval);
          }
          
        }, 50);
        break;
      }
    }
    return 0;
  }
  
  
}

const animation_tank_left = new Animation_Movement_Tank(50, 50, 0, document.getElementById('tank'));
const animation_tank_right = new Animation_Movement_Tank(50, 50, 1, document.getElementById('tank'));
const animation_tank_top = new Animation_Movement_Tank(50, 50, 2, document.getElementById('tank'));
const animation_tank_down = new Animation_Movement_Tank(50, 50, 3, document.getElementById('tank'));

Chodzi o to, że wiesz, że np. animacja będzie zawsze co 50 pikseli, więc powinieneś przesuwać tylko offset, a nie wszystko. Czyli włączasz animację, jeśli dojdzie do końca skaczesz, co 50. Następna animacja jedzie od 0, wiec już skoczyłeś 50, więc animacja zaczyna od 50 i dalej jedzie o 50 jednostek do przodu, czyli to będzie już 100.

Załączasz sobie raz 4 animacje ich nie liczysz, tylko po zakończeniu animacji wykonujesz skok, żeby animacja była zaczynana relatywnie w innym miejscu.

Żeby obliczyć np. 5 kwadratów (bloków) o wymiarach 50x50, to musiałby skrypt liczyć 50*5 = 250 jednostek np. pętlą. W mojej metodzie będziesz mieć 4*50 = 200 jednostek pętlą, tylko na początku skrypt to oblicza, żeby później mieć wolne, dla animacji "lewo", "prawo", "góra", "dół". I później dla każdego wywołania jak dokończysz ten kod będziesz miał stałą 200 plus tutaj 5, bo skacze co 50, więc razem to będzie 205.

Widzisz różnicę chyba. Nie powinno tak mulić przy każdym naciśnięciu myszą przy renderingu. Dla Twojej metody pętla potrzebuje 250 obrotów, w mojej 205, ale im dalej w las, to tym Twoja metoda zaczyna być bardzo mało wydajna.

Wzór na Twoją metodę (width_or_height*amount_of_blocks), wzór dla mojej metody (width_or_height*4)+amount_of_blocks.

Jeśli się nigdzie nie pomyliłem w zamyśle, to tak to powinno mniej więcej wyglądać.

Gdy czołg miałby przejechać z punktu 0*50 = 0, 0*50 = 0 do punktu 20*50 = 1000, 20*50 = 1000, to wtedy. 50, to szerokość i wysokość bloku. width_or_height = 50, amount_of_blocks = 40, bo poziomo 20 i pionowo 20.

Twoja metoda: 50*40 = 2000 obrotów pętlą

Moja metoda (50*4)+40 = 240 obrotów pętlą

komentarz 15 lutego 2020 przez obl Maniak (51,120 p.)
Wciąż nie rozumiem o co ci chodzi. Mam mapę na mapie są różne obiekty, każdy obiekt tak czy siak muszę odrysować bo muszą się wykonać przemieszczenia pocisków, czołgu, wieżyczek. Przemieszczanie się odbywa do punktu. Nie ma żadnej oszczędności w tym co piszesz  Odrysowanie zawsze odbywa się na jeden cykl odświeżenia widoku przeglądarki i polega na przerysowaniu jednej grafiki na canvas twoja metoda to samo by robiła. To przeglądarka odświeża widok co 1/60 sekundy. A na to odświeżanie wykonuje się odrysowanie i logika animacji.Czołg to jest grafika zależna od kierunku przemieszczania obrócona odpowiednio, gdy ma się przemieszczać dostaje wektor ruchu. Ma stanąć wektor jest zerowy. Na bardzo starym 32 bitowym i słabym kompie uruchomiłem i tam przymulało ale przyznam szczerze to jest Angular i on sam w sobie przymulać też może. Obliczanie animacji jest mało czasochłonne. To odrysowywanie jest tym co przymula i tu musiałbym poczynić optymalizację.
komentarz 16 lutego 2020 przez obl Maniak (51,120 p.)

@Chess,
W sumie miałeś rację że z optymalizacją jest krucho. Nie zauważałem tego na moim kompie bo zbyt mocny był ale zapomniałem o jednej optymalizacji którą na koniec zostawiałem. Teraz już jest znacznie lepiej bo odrysowuję tylko te obiekty, które się mieszczą w okienku.

Dzięki za odzew i rady. Nie ma to jak ktoś zerknie i wytknie błędy to można wtedy popracować nad dopracowaniem projektu.

1 odpowiedź

+1 głos
odpowiedź 16 lutego 2020 przez Chess Szeryf (76,750 p.)
wybrane 17 lutego 2020 przez obl
 
Najlepsza
<body style="background: gray; margin: 10px; padding: 0;">

Animation only width and height block e.g. 50 x 50 and jump what 50 unit.
<pre>
[(==)|    ]
[    |(==)]
</pre>
<div id="container" style="position: relative; background: lightblue; height: 50px; width: 100px; background: transparent;">
  <div id="tank" style="background: green; position: absolute; width: 50px; height: 50px; left: 0px; top: 0px;"></div>
  <div id="no_visible" style="background: grey; width: 50px; height: 50px; z-index: 34; position: absolute; left: 100px;"></div>
</div>
<div id="container2" style="position: relative; background: lightblue; height: 50px; width: 110px;">

<div id="tank2" style="background: lightgreen; position: absolute; width: 50px; height: 50px; left: 0px; top: 0px;"></div>
</div>
class Animation_Movement_Tank {
  constructor(width, height, direction, tank) {
    this.width = width;
    this.height = height;
    this.direction = direction;
    this.tank = tank;
    this.flag = -1;
    this.i = 0;
  }
  
  animate_frames(offset, arg1) {
    //document.getElementById('no_visible').style.visibility = "visible";
    let promise1;
    
    switch (this.direction) {
      case 0: {
        this.i = 0; //this.width - 1;
        promise1 = (new Promise(resolve => {
          //i = 0;
          let clear_interval = setInterval(() => {
         
          this.tank.style.left = 0 + this.i + "px";
          
          //arg1 == '+' ? this.i++ : this.i--;
          this.i--;
          document.getElementById('no_visible').style.background = "green";
          document.getElementById('no_visible').style.visibility = "hidden";
          //document.getElementById('no_visible').style.display = "none";
   
          
          //d//ocument.getElementById('no_visible').style.background = 'green';
          console.log(this.i, this.width, offset);
          if(offset[1] == false && this.i <= -50 - 1 || offset[1] == true && this.i <= -50 - 1 - offset[0] + 1) { // +1
            this.flag = 0;
            
            resolve([1, this.i]);
            clearInterval(clear_interval);
          }
          }, 50);
          
        }));
        break;
      }
      case 1: {
        //let i = 0;
        
        this.i = 0;
        
        promise1 = (new Promise(resolve => {
          //i = 0;
          let clear_interval = setInterval(() => {
         
          this.tank.style.left = 0 + this.i + "px";
          
          //arg1 == '+' ? this.i++ : this.i--;
          this.i++;
          document.getElementById('no_visible').style.background = "green";
          document.getElementById('no_visible').style.visibility = "hidden";
          //document.getElementById('no_visible').style.display = "none";
   
          
          //d//ocument.getElementById('no_visible').style.background = 'green';
          console.log(this.i, this.width, offset);
          if(offset[1] == false && this.i >= this.width + 1 || offset[1] == true && this.i >= this.width + 1 - offset[0] - 1) { // +1
            this.flag = 0;
            
            resolve([1, this.i]);
            clearInterval(clear_interval);
          }
          }, 50);
          
        }));
        break;
      }
      case 2: {
        let i = this.height - 1;
        var clear_interval = setInterval(function() {
          
          this.tank.style.top = offset + i + "px";
          i--;
          if(i <= 0) {
            this.flag = 0;
            clearInterval(clear_interval);
          }
          
        }, 50);
        break;
      }
      case 3: {
        let i = 0;
        var clear_interval = setInterval(() => {
          
          this.tank.style.top = offset + i + "px";
          i++;
          
          if(i >= this.height - 1) {
            this.flag = 0;
            clearInterval(clear_interval);
          }
          
        }, 50);
        break;
      }
    }
    //return this.promise1.then((val) => {
      //return this.promise1;
    //});
    return promise1;
  }
  
  
}

const animation_tank_left = new Animation_Movement_Tank(50, 50, 0, document.getElementById('tank'));
const animation_tank_right = new Animation_Movement_Tank(50, 50, 1, document.getElementById('tank'));
const animation_tank_top = new Animation_Movement_Tank(50, 50, 2, document.getElementById('tank'));
const animation_tank_down = new Animation_Movement_Tank(50, 50, 3, document.getElementById('tank'));

//const test_animation = new Animation_Movement_Tank(50, 50, 1, document.getElementById('tank2'));

let i = 0;
let coll = -10; // 5
let offset = 5;
const signs = ["-", "+"];
let j = 0;
const directions_arr = [[1, 3], [0, 2]]; // 2 and 1
let all = 0;
let ii = 0;
let factor_coefficient = 1;

function direction(arg, a, b) {
  if(arg == 0 && a > b) {
    return true;
  } else if(arg == 1 && a < b) {
    return true;
  }
  return false;
}

function recursion1(info, info2) {
  if(info[1] == true) {
    console.log(22);
    info2 = directions_arr[j][0];
    j++;
    info[1] = false;
    ii = 0;
  }
  //for(let j=0;j<signs.length;j++) {    
  console.log([animation_tank_left, animation_tank_right][info2].animate_frames(info).then(function(value) {
    //for(let i=0;i<1;i++) {

      //console.log( document.getElementById('container').style.marginLeft);
      
      //document.getElementById('container').style.marginLeft = offset + coll + "px";
      all++;
      
      if(info2 == 0) {
        i--;
        ii--;
        factor_coefficient = -1;
      } else if(info2 == 1) {
        i++;
        ii++;
        factor_coefficient = 1;
      }
      coll = i;
      
      //coll *= -1;
      console.log(all, ii);
      //offset = (50*i);
      console.log(info2, i, 4);
      if(value[0] == 1 && all < 7) {
        //document.getElementById('tank').style.background = 'grey';
        //document.getElementById('no_visible').style.visibility = 'hidden';
        
        document.getElementById('no_visible').style.left = (50*factor_coefficient) + "px";
   
        document.getElementById('container').style.marginLeft = ((50*i)+coll*1) + "px"; // 5 value[1]
        //document.getElementById('tank').style.background = 'pink';
        //console.log(i);
        
        //coll *= -1;
        document.getElementById('no_visible').style.visibility = "visible"; // <--
        document.getElementById('no_visible').style.background = "grey"; // <--
        //document.getElementById('no_visible').style.display = 'block'; // <--
        
        //offset = 0;
        //value[0] = -1;
        //document.getElementById('no_visible').style.visibility = "hidden";
        recursion1([coll*1, j < directions_arr.length && ii == directions_arr[j][1] - 1 ? true : false], info2);
      }
    //}
  }));
//}
}
// run maybe in the same time, tank2 later usually
recursion1([0, false], 1);

let x = 0;
/*
var cleared = setInterval(() => {
document.getElementById('tank2').style.left = x + 'px';
  x++;
  if(x >= 200) {
    clearInterval(cleared);
  }
}, 50);
*/
</script>
</body>

Myślę, że będziesz wiedział jak dokończyć. Skorygowanie pikseli i miganie, gdy kwadrat (czołg) przejedzie 50. piksel. case 2 i 3 powinieneś analogicznie napisać do case 0 i 1.

Podobne pytania

+1 głos
1 odpowiedź 148 wizyt
+2 głosów
2 odpowiedzi 380 wizyt
pytanie zadane 28 sierpnia 2018 w Nasze projekty przez mokebe Nowicjusz (210 p.)
+1 głos
0 odpowiedzi 100 wizyt
pytanie zadane 9 kwietnia 2021 w PHP przez Lopus Początkujący (360 p.)

87,946 zapytań

136,526 odpowiedzi

304,410 komentarzy

58,313 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Sklep oferujący ćwiczenia JavaScript, PHP, rozmowy rekrutacyjne dla programistów i inne materiały

Oto dwie polecane książki warte uwagi. Pełną listę znajdziesz tutaj.

...