Quaxli Tutorial = ConcurrentModificationException

[truesoul]

Hallo miteinander,

also mein Problem ist das ich eine java.util.ConcurrentModificationException.
Dieses Problem ist zuvor nie aufgetretten und tritt jetzt ohne änderung am Code auf.

Also der Fehler tritt in der paintComponent() Methode auf :

public void paintComponent(Graphics g){
        super.paintComponent(g);

        g.drawImage(background, 0, 0, this);

        g.setColor(Color.red);
        g.drawString("FPS: " + Long.toString(fps), 20, 10);

        if(!isStarted()){
            return;
        }
        

        if(actors != null){
            for(Drawable draw:actors){ // HIER TRITT DER FEHLER AUF!!!
                draw.drawObjects(g);
            }
        }
    }

Das ist die Klasse GamePanel

/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */

package helix;

import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.awt.image.BufferedImage;
import java.io.IOException;
import java.net.URL;
import java.util.Vector;
import javax.imageio.ImageIO;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.Timer;

public class GamePanel extends JPanel implements Runnable , KeyListener, ActionListener{

    private static final long serialVersionUID = 1L;

    private JFrame frame;

    boolean started = false;
    boolean once = false;

    long delta = 0;
    long last = 0;
    long fps = 0;
    long gameover = 0;

    //Steuerung
    boolean up = false;
    boolean down = false;
    boolean left = false;
    boolean right = false;
    int speed = 70;

    Timer timer;
    BufferedImage[]rocket;
    BufferedImage background;

    Heli copter;
    Vector<Sprite> actors;

    public GamePanel(int w , int h)
    {
        this.setPreferredSize(new Dimension(w, h));
        frame = new JFrame("GameDemo");
        frame.setLocation(100, 100);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.addKeyListener(this);
        frame.add(this);
        frame.pack();
        frame.setVisible(true);
        doInitializations();
    }

    private void doInitializations()
    {

        BufferedImage[] heli = loadPics("pics/Helix.gif", 5);
        rocket = loadPics("pics/Rockets.gif", 8);
        background = loadPics("pics/BergHimmel.jpg", 1)[0];

        last = System.nanoTime();
        gameover = 0;
        actors = new Vector<Sprite>();
        copter = new Heli(heli, 400, 300, 100, this);
        actors.add(copter);


        timer = new Timer(3000, this);
        timer.start();

        if(!once){
           Thread t = new Thread(this);
            t.start();
        }
        
    }

    public void run(){

        while(frame.isActive()){

            computeDelta();
            
            if(isStarted())
            {
                //checkKeys();
                doLogic();
                moveObjects();
            }
            
            repaint();

            try{
                Thread.sleep(10);
            }catch ( InterruptedException e){}
        }
    }

    private void createRocket(){

        int x = 0;
        int y = (int)(Math.random()*getHeight());
        int hori = (int)(Math.random()*2);

        if(hori == 0){
            x = -30;
        }else{
            x = getWidth()+30;
        }

        Rocket rock = new Rocket(rocket, x, y, 100, this);
        if(x<0){
            rock.setHorizontalSpeed(100);
        }else{
            rock.setHorizontalSpeed(-100);
        }
        actors.add(rock);
    }

    private void doLogic(){
        Vector<Sprite> trash = new Vector<Sprite>();

        for(Movable mov:actors){
            mov.doLogic(delta);
            Sprite check = (Sprite)mov;
            
            if(check.remove){
                trash.add(check);
            }
        }

        for(int i = 0; i < actors.size();i++){
            for( int n = i+1; n < actors.size(); n++){
                Sprite s1 = actors.elementAt(i);
                Sprite s2 = actors.elementAt(n);

                s1.collidedWith(s2);
            }
        }

        if(trash.size()>0){
            for(Sprite s : trash){
                actors.remove(s);
            }
        }

        if(gameover > 0){
            if(System.currentTimeMillis() - gameover>3000){
                stopGame();
            }
        }
    }

    private void stopGame(){
        timer.stop();
        setStarted(false);
    }

    private void moveObjects(){
        for(Movable mov:actors){
            mov.move(delta);
        }
    }



    private BufferedImage[] loadPics(String path , int pics){

        BufferedImage[] anim = new BufferedImage[pics];
        BufferedImage source = null;

        URL pic_url = getClass().getResource(path);

        try{
            source = ImageIO.read(pic_url);
        }catch ( IOException e){}

        for(int x = 0; x < pics;x++){
            anim[x] = source.getSubimage
                    (
                        x*source.getWidth()/pics,
                        0,
                        source.getWidth()/pics,
                        source.getHeight()
                    );
        }

        return anim;
    }

    private void computeDelta()
    {
        delta = System.nanoTime() - last;
        last = System.nanoTime();

        fps =((long) 1e9)/delta;
    }

    public void paintComponent(Graphics g){
        super.paintComponent(g);

        g.drawImage(background, 0, 0, this);

        g.setColor(Color.red);
        g.drawString("FPS: " + Long.toString(fps), 20, 10);

        if(!isStarted()){
            return;
        }
        
        if(actors != null){
            for(Drawable draw:actors){
                draw.drawObjects(g);
            }
        }
    }

    public boolean isStarted(){
        return started;
    }

    public void setStarted(boolean started){
        this.started = started;
    }
}

Der Fehler tritt erst dann auf wenn der Helicopter mit der Rakete zusammengestoßen ist und die beiden Objekte entfernt wurden.


Was habe ich übersehen bzw warum wird diese Exception ausgelöst?!?

Programm Code

Danke schonmal im Vorraus.

Mfg

 

[Gast2]

            for(Sprite s : trash){
                actors.remove(s);
            }

            for(Drawable draw:actors){ // HIER TRITT DER FEHLER AUF!!!
                draw.drawObjects(g);
            }

Diese beiden schleifen laufen parallel und greifen beide auf actors zu. Wenn du jetzt in der oberen Schleife was aus actors entfernst, dann fliegt die Exception.
Entweder du verwendet überall nen Iterator und löschst über den dann deine Elemente oder du probierst es mal mit ner synchronisierten liste. (würde aber erst mal ersteres versuchen).

 

[SlaterB]

> for(Drawable draw:actors){
reagiert empfindlich wenn die Liste zwischendurch geändert wird,
bei nebenläufigen Zeichnen + Ändern der Daten kann das schon passieren,

> Entweder du verwendet überall nen Iterator und löschst über den dann deine Elemente

ein Iterator kommt mit sich selber zurecht, bei zweien bleibt das Problem bestehen, andere Änderungen sind schlecht, egal von wem,
wenn dann müsste es schon ein sehr spezieller Iterator sein, dessen Instanzen alle miteinander verknüpft sind, sich gegenseitig informieren

-----

eine Patentlösung gibts dazu wohl nicht, wenn ich jetzt mal drüber nachdenke,

die Spiel-Verarbeitung während des Zeichnens komplett zu pausieren klingt wenig sinnvoll,
sofern man überhaupt feststellen kann wann gezeichnet wird, paintComponent müsste sich dazu zu Beginn und Ende melden,
andererseits hat man dann auch nicht das Problem, dass seltsame Dinge gezeichnet werden, weil ein Objekt noch den alten Zustand hat, ein anderes schon den neuen

besser ist schon wenn paintComponent sich die Daten kopiert, bevor es sie durchläuft,
aber das ist aufwendig und die Exception kann dann immer noch kommen, bei Änderung genau zum Zeitpunkt des Kopierens

 

[Flown]

Vorher:

for(Movable mov:actors){
            mov.doLogic(delta);
            Sprite check = (Sprite)mov;
            
            if(check.remove){
                trash.add(check);
            }
        }

        for(int i = 0; i < actors.size();i++){
            for( int n = i+1; n < actors.size(); n++){
                Sprite s1 = actors.elementAt(i);
                Sprite s2 = actors.elementAt(n);

                s1.collidedWith(s2);
            }
        }
        if(trash.size()>0){
            for(Sprite s : trash){
                actors.remove(s);
            }
        }

Nacher/Richtig?:

for(Movable mov:actors){
            mov.doLogic(delta);
            Sprite check = (Sprite)mov;
            
            if(check.remove){
                trash.add(check);
            }
        }
        if(trash.size()>0){
        	for(Sprite s : trash){
        		actors.remove(s);
        	}
        }

        for(int i = 0; i < actors.size();i++){
            for( int n = i+1; n < actors.size(); n++){
                Sprite s1 = actors.elementAt(i);
                Sprite s2 = actors.elementAt(n);

                s1.collidedWith(s2);
            }
        }

Du zeichnest sie bevor du sie löscht!

 

[Kr0e]

Nimm Collections aus java.util.concurrent.* Z.b. ConcurrentLinkedQueue, da gibt es solche Exceptions nicht.
Da kannst du Löschen wann immer du willst...

 

[Flown]

private synchronized void doLogic(){
...
}

Also mit dem hab ich gerade keine Exception mehr bekommen. Versuchs mal...

EDIT:

private synchronized void moveObjects(){
...
}

 

[truesoul]

@Flown

Nope , der Fehler kommt weiterhin.
EDIT: Versuche es eben mit synchronized, mom
EDIT EDIT: Nope , kommt weiterhin.

@EikeB

Habe jetzt jede trash und actors je ein Iterator gegeben und gehe so durch.

Iterator<Sprite> it = actors.iterator();
            while(it.hasNext()){
                Drawable draw = it.next();
                draw.drawObjects(g);
            }

// anstatt so:

 for(Drawable draw:actors){ // HIER TRITT DER FEHLER AUF!!!
                draw.drawObjects(g);
            }

und ...

Iterator<Sprite> it = trash.iterator();
        if(trash.size()>0){
            while(it.hasNext())
            {
                Sprite s = it.next();
                actors.remove(s);
            }
        }

// anstatt so:

for(Sprite s : trash){
                actors.remove(s);
            }

Alle zugriffe auf actors habe ich mit Iterator ersetzt.
Leider kommt weiterhin der fehler.

@Kr0e
Versuche es mal mit deinem Vorschlag, denke mit peek(); greife ich immer auf das nächste Element drauf zu , genau ? bzw Hole mir das nächste Element.

@SlaterB
Hätte nicht erwartet das sowas relative einfaches doch komplizierter werden kann als man dachte.

 

[truesoul]

Habe jetzt auch:

public synchronized void paintComponent(Graphics g)
{
       .... 
}

Da kam dann nach längerem ausprobieren keine Exception, allerdings ist das Spiel am Ruckeln.
Das ist natürlich nicht sehr berauschend

Versuche es eben mit ConcurrentLinkedQueue.

 

[Marco13]

ConcurrentLinkedQueue klingt komisch. Wenn schon eine der Concurrent-Collections, dann hätte ich jetzt eher an eine CopyOnWriteArrayList gedacht - dürfte einfacher sein und weniger Änderungen nach sich ziehen, weil's ja auch "nur" eine List ist.

Ansonsten könnte man die Liste auch von Hand synchronisieren. Ganz grob nach diesem Muster

private List<Actor> actors = Collections.synchronizedList(new ArrayList<Actor>());

...

void doLogic()
{
    List<Actor> toRemove = new ArrayList<Actor>();
    for(Movable mov:actors)
    {
        if (hasToBeRemoved(mov)) toRemove.add(mov);
    }
    actors.removeAll(toRemove); // Thread safe weil synchronizedList
}

public void paintComponent(...)
{
    synchronized (actors) // Snychronisieren zum Drüberiterieren
    {
        for(Drawable draw:actors){ // HIER TRITT DER FEHLER (jetzt hoffentlich nicht mehr ;-) ) AUF!!!
                draw.drawObjects(g);
        }    
    }
}

 

[truesoul]

Hallo Marco ,

danke schonmal für deine Hilfe.

Habe jetzt mal deine Variante getestet. Also ich bekomme jetzt keine Exception mehr allerdings Ruckelt das Spiel extrem sowie bei der Variante mit

 public synchronized void paintComponent(Graphics g)

?!?

Hier der neue Code:
Programm Code

EDIT: Probiere es eben aus mit CopyOnWriteArrayList.
Danke schonmal für den Hinweis.

EDIT: Also mit CopyOnWriteArrayList würde es auch gehen, aber trotzdem besteht weiterhin das Problem mit dem Ruckeln

 

[Flown]

Also ich habs eben getestet und bei mir ruckelt gar nichts ...

 

[truesoul]

@Flown
Also ich habe es eben nochmal getestet.
Es kann seine Zeit dauern, aber irgendwann geht die CPU-Auslastung auf 100% und dann ruckelt extrem was bei 100% auslastung kein Wunder ist

EDIT:
Habe jetzt den repaint() Aufruf in der run() Methode im if-Block rein gepackt, da er nur Zeichnen soll, wenn das Spiel Aktiv ist. Also started == true.

if(isStarted()){

     checkKeys();
     doLogic();
     moveObjects();
     repaint();
}

Damit habe ich schonmal etwas gewinn machen können.
Aber was mir aufgefallen ist das nach längeren Spielen (2-3 Minuten) die CPU Auslastung hoch geht, und dann permanent oben bleibt. Sobald ich dann Enter drücke für starten, geht er auf 90% hoch.
Wird der Heli getroffen geht er wieder auf ca. 60%, aber vor den 2-3 Minuten ging er immer ca. 12 % und weniger.

Also als ob er in einer Schleife hängt , tut er aber nicht?!?

 

[Quaxli]

Das Perverse an der Geschichte ist, daß diese Exception bei mir nicht geworfen wird. Und ich weiß ums Verrecken nicht warum. ;(;(;( Wenn mir aber jemand seinen Code schickt, wirft der auch bei mir die Exception.
Das bedeutet, daß es einen Unterschied geben muß, aber ich weiß schon nicht mehr was ich noch vergleichen soll.

Ich habe an dem Problem auch schon rumgebastelt und mit synchronized und diversen anderen Sache rumexperimentiert. Lt. API ist Vector an sich ja eigentlich synchronized.
Nach ein bißchen Rumsuchen habe ich den Hinweis gefunden, daß der Bösewicht wohl die foreach-Schleife ist.

Momentaner Lösungsvorschlag von mir:

Statt z. B.:

		for(Movable mov:actors){
			mov.move(delta);
		}

iteriert wie man wie folgt über die Collection:

		for(int i=0;i<actors.size();i++){
			actors.get(i).move(delta);
		}

Damit sollte das Problem behoben sein, ohne daß es Performance-Probleme gibt.
Ich bastel da aber momentan noch dran rum und überlege, ob es bessere Möglichkeiten gibt.

In der aktuellen Version des Tutorials ist der Hinweis auf JavaConcurrentException übrigens auch drin (seit dem Wochenende).

 

[SlaterB]

behoben im Sinne von stark verringert,
prüft Thread1 actors.size() am ende der Liste und greift noch nicht auf das Element zu
sondern entfernt zwischenzeitig erst Thread2 ein Element,
dann könnte in Thread1 actors.get(i) eine Exception werfen, da es kein entsprechendes Element gibt

 

[truesoul]

Hallo Quaxli ,

ich habe zwischenzeitlich auch mit der

for(int i=0;i<actors.size();i++){
            actors.get(i).move(delta);
        }

Variante versucht.
Es verringert den fehler tatsächlich aber er kann leider noch immer auftretten.
P.S Diese Variante hab ich auch irgendwo hier als Thema gefunden gehabt .

Und wie SlaterB schon in seinem Post schreibt kann es eine Exception schmeißen wenn Thread2 ein Element entfernt und Thread1 auf das Element , das entfernt wurde , zugreifen will.
Ist einmal vorgekommen, bei meinen bisherigen tests

Was einigermaßen gut den fehler verhindert ist tatsächlich

 synchronized

und

 for(int i=0;i<actors.size();i++)

aber dennoch tritt er auf oder es kommt die besagt Exception (siehe SlaterB)

Im übrigen schau ich auch nach ein anderen Weg, wie man das lösen könnte, für Tipps wäre ich dankbar.

EDIT:
Das führt übrigens auch zu diesem Fehler:

        List<String> list = new ArrayList<String>( Arrays.asList( "Hallo", "du" ) );
        Iterator<String> iterator = list.iterator();
        System.out.println( list.get( 0 ) );  // Hallo
        list.add( "Entspannung" );
        iterator.next();

 

[Marco13]

Habe jetzt mal deine Variante getestet. Also ich bekomme jetzt keine Exception mehr allerdings Ruckelt das Spiel extrem sowie bei der Variante mit

 public synchronized void paintComponent(Graphics g)

?!?
...
EDIT: Also mit CopyOnWriteArrayList würde es auch gehen, aber trotzdem besteht weiterhin das Problem mit dem Ruckeln

Zwei Threads greifen auf die gleiche Liste zu. Einer von beiden verändert die Liste. Man hat die Optionen:
- Exceptions durch gleichzeitigen Zugriff
- Ruckeln wenn man Synchronisiert
Exceptions durch gleichzeitigen Zugriff kann man nicht verhindern. (Punkt.). In irgendeiner Form MUSS man die Threadsicherheit herstellen. Sei es durch synchronisation auf Ebene der Methoden oder auf Ebene der Liste, oder durch eine Datenstruktur, die die snychronisation intern (und dort wieder durch eigene synchronizeds, Locks & Conditions oder Kopieren) regelt.
Auch ein Ruckeln ist bei so einem Spiel nicht aktzeptabel. Also bleibt m.E. nur: Man versucht, die effizienteste Synchronisationsmethode zu finden, und wenn es dann immernoch ruckelt, überlegt man vielleicht mal, ob man das Thread.sleep(10) zu einem Thread.sleep(20) machen könnte, damit er nicht 100 mal pro Sekunde versucht, die Logik durchzuarbeiten und neu zu zeichnen.

 

[Quaxli]

Ich denke z. Z. darüber nach, ob aktives Rendern das Problem lösen könnte. Wenn man selbst das Zeichnen übernimmt (sozusagen), sollte das Problem auch zu eliminieren sein. Ich werde das bei Gelegenheit mal antesten.

 

[Flown]

Und was ist wenn man das überhaupt anders aufzieht z.B. in einem MVC-Modell? Wäre das vielleicht besser mit einem zentralen repaint und das Modell selbst übernimmt die Logik?

 

[Quaxli]

Probier es aus. Und wenn's klappt: Schreib ein Tutorial.

Wie schon im Tutorial steht: Es ist (m)ein Weg.
(Nicht das 11. Gebot der Spieleprogrammierung).

 

[Flown]

Wie schon im Tutorial steht: Es ist (m)ein Weg.
(Nicht das 11. Gebot der Spieleprogrammierung).

Aber es funktioniert imho bei (fast) allen Fällen problemlos.

 

[truesoul]

Hallo miteinander,

verstehe ich das MVC model richtig , das im Model die Logik & Listen von den zum Beispiel Raketen , Wolken oder Helicopter stehen?
Im Controller dann die Listener?
Und View dann das Zeichnen?


Habe ehrlich gesagt noch nicht wirklich damit was zu tun gehabt und erst eben ne 3/4 Stunde paar beispiele gelesen. :rtfm:

Beispiele in bezug MVC und Zeichnen auf JPanels oder Canvas wären mir natürlich auch ganz recht
Selber suche ich natürlich nach solch beispielen.

Versuche grad auch die Ruckler zu beseitigen.
@Marco
Dein Rat mit von 10ms auf 20 ms hat schon ein gefüllten erfolg gebracht, nur die CPU Auslastung ist noch immer recht hoch

Danke schonmal für die beteiligung an diesen Problem.

Mfg

 

[Flown]

Ne gute Beschreibung findest du eben auch unter den FAQ's.

 

[truesoul]

Danke, werde mich direkt ran setzen

 

[Marco13]

Wobei so ein "Echtzeitspiel" nicht gerade das klassische Anwendungsgebiet für MVC ist... Ansätze, die in diese Richtung gehen und einige Grundsätze vom MVC berücksichten, ja, vielleicht, aber das ganz pragmatische Runterschreiben eines MVC könnte da nicht sooo viele Vorteile bringen.

 

[truesoul]

Wie wäre es denn, wenn man die Objekte garnicht erst aus der Liste entfernt sondern sozusagen nicht mit zeichnet?

Ich erstelle eine Liste aller Objekte die im Spiel vorkommen sollen.

Liste 0-9 == Wolken
Liste 10-13 == Raketen
Liste 14 == Helicopter
Liste 15 == Explosion
usw

Beim Spielstart werden Wolken, Raketen und Helicopter gezeichnet und sobald ein Helicopter+Rakete oder Rakete+Rakete zusammenstoßen weiß ich ja wo in der Liste die sich befinden und beim neu Zeichnen werden diese Positionen ausgelassen und dafür die Explosion gezeichnet.
Und dann erst wird wieder alles auf default gesetzt und Helicopter und Raketen wieder mitgezeichnet.
Bei dem was ich jetzt habe also nach Enter???

Ist nur eine naive Idee

Mfg

 

[Quaxli]

Mir ist noch eine weitere Lösungsmöglichkeit eingefallen: Man muß ja eigentlich nur sicherstellen, daß kein 2. Thread auf die Collection zugreift.
Man könnte daher ein Image bereitstellen, in das der GameLoop rein zeichnet. Dann greift nur ein Thread auf die Collections zu, während der 2. nur das Bild zeichnet.

 

[Marco13]

Hm. Klingt für mich nach einem fragwürdigen Workround ... und ... willst du gegenüber der Synchronisation von ZWEI lächerlichen Threads kapitulieren? :noe:
Ich denke, das hätte den Nachteil dass die zum Zeichnen benötigte Zeit und die für die Spiellogik benötigte Zeit stark gekoppelt sind: Wenn die Logik 10ms dauert und das Zeichnen dann 90ms, dann muss der Gamethread mit "riesigen" Zeitschrittweiten von 100ms klarkommen. Wenn man beides trennt könnte das Spiel mit 10ms-Schritten laufen, und immerhin noch mit ca. 10 FPS rendern.

 

[Quaxli]

So betrachtet....
.... hast Du natürlich auch wieder Recht.

 

[SlaterB]

also wenn man nicht gerade mehrere Kerne arbeiten läßt, dann wird 5x 10ms Spiel pro 100ms die FPS-Rate auch drastisch kürzen,
die 90ms zum Zeichnen kann man rechnerisch ja gar nicht in restlichen 50ms der 100ms quetschen

was soll das Spiel eigentlich leisten in diesen 10ms zwischendurch?
welches Update ist dann schon nötig statt erst genau vorm nächsten Zeichnen,
wenn alle Aktionen mit Zeitpunkten ausstatten, kann man auch nach 100ms feststellen, dass xy bereits seit 70 ms in neuen Zustand z ist

 

[Marco13]

also wenn man nicht gerade mehrere Kerne arbeiten läßt, dann wird 5x 10ms Spiel pro 100ms die FPS-Rate auch drastisch kürzen,
die 90ms zum Zeichnen kann man rechnerisch ja gar nicht in restlichen 50ms der 100ms quetschen

Hm. Das bezog sich auf eine Architektur, bei der Zeichnen und Logik wirklich entkoppelt sind. Wo er also vielleicht 10 Schritte mit je 10ms macht, aber dann nur einmal zeichnet (eben alle 100ms). Da wär synchronisation natürlich Sch.... lecht, da müßte man irgendwie Puffern: "Eine Kopie des aktuellen Zustands soll gezeichnet werden (egal wie lange du dafür brauchst, ich rechne derweil vielleicht 5 oder 10 neue Zustände aus, die nicht gezeichnet werden)"...

 

[Helgon]

Hatte das Problem auch, wenn die Logik Überprüfung zu lange dauert (und von woanders vorschnell drauf zugegriffen wird). Einfach ne dritte Kopie und es hat sich