Active Tile not moving even though I am changing it's position

[Doogledude123]

I'm making one of those puzzle games that has the empty tile which you need to slide them around to finish the picture. When I refer to the "Active Tile" I'm referring to the tile that's empty on the board.

I'm changing the position of the active tile and the tile that's one position in the opposite direction of the directional keys that were pressed (doing this to simulate sliding), however, when I press a key to move them, the active tile is still in the same position as before (it will not move).

Here's all the relavent code:

package dev.geodox.shuffle.gfx;

import java.awt.*;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.awt.image.BufferedImage;
import java.util.*;
import java.util.List;

import javax.swing.JPanel;

import dev.geodox.shuffle.helper.Difficulty;

@SuppressWarnings("serial")
public class ShuffleScreen extends JPanel
{
    private Dimension windowSize;
    private Random shuffler;
    private BufferedImage image;
    private Difficulty diff;
    private Tile[][] tiles;
    private Tile activeTile;
    public int diffWidth;
    public int diffHeight;
    private int tileWidth;
    private int tileHeight;
    private int tileOffsetX;
    private int tileOffsetY;

    public ShuffleScreen(BufferedImage image, final Difficulty diff)
    {
        this.image = image;
        this.diff = diff;

        diffWidth = diff.getWidth();
        diffHeight = diff.getHeight();

        tiles = new Tile[diffWidth][diffHeight];
        tileWidth = image.getWidth() / diffWidth;
        tileHeight = image.getHeight() / diffHeight;

        addKeyListener(new KeyListener()
        {
            @Override
            public void keyTyped(KeyEvent e)
            {

            }

            @Override
            public void keyPressed(KeyEvent e)
            {

            }

            @Override
            public void keyReleased(KeyEvent e)
            {
                if(e.getKeyCode() == KeyEvent.VK_LEFT)
                {
                    if(activeTile.getPosX() > 0)
                    {
                        swap(tiles[activeTile.getPosX() + 1][activeTile.getPosY()], activeTile);
                        repaint();
                    }
                }
                else if(e.getKeyCode() == KeyEvent.VK_RIGHT)
                {
                    if(activeTile.getPosX() < diffWidth)
                    {
                        swap(tiles[activeTile.getPosX() - 1][activeTile.getPosY()], activeTile);
                        repaint();
                    }
                }
                else if(e.getKeyCode() == KeyEvent.VK_UP)
                {
                    if(activeTile.getPosY() > 0)
                    {
                        swap(tiles[activeTile.getPosX()][activeTile.getPosY() + 1], activeTile);
                        repaint();
                    }
                }
                else if(e.getKeyCode() == KeyEvent.VK_DOWN)
                {
                    if(activeTile.getPosY() < diffHeight)
                    {
                        swap(tiles[activeTile.getPosX()][activeTile.getPosY() - 1], activeTile);
                        repaint();
                    }
                }
            }
        });

        splitImage();
    }

    private void splitImage()
    {
        for(int i = 0; i < diff.getWidth(); i++)
        {
            for(int j = 0; j < diff.getHeight(); j++)
            {
                tiles[i][j] = new Tile(image.getSubimage(i * tileWidth, j * tileHeight, tileWidth, tileHeight), diff, tileWidth, tileHeight);
                tiles[i][j].setPos(i, j);
            }
        }
    }

    public void shuffle()
    {
        int toRemoveX = 0, toRemoveY = 0;
        shuffler = new Random();

        Collections.shuffle(Arrays.asList(tiles));
        for(int i = 0; i < tiles.length; i++)
            Collections.shuffle(Arrays.asList(tiles[i]));

        for(int i = 0; i < tiles.length; i++)
        {
            for(int j = 0; j < tiles[0].length; j++)
            {
                tiles[i][j].setPos(i, j);
            }
        }

        toRemoveX = shuffler.nextInt(tiles.length);
        toRemoveY = shuffler.nextInt(tiles[0].length);

        activeTile = tiles[toRemoveX][toRemoveY];

        revalidate();
    }

    private void swap(Tile from, Tile to)
    {
        tiles[from.getPosX()][from.getPosY()].setPos(to.getPosX(), to.getPosY());
        tiles[from.getPosX()][from.getPosY()] = to;
        tiles[to.getPosX()][to.getPosY()].setPos(from.getPosX(), from.getPosY());
        tiles[to.getPosX()][to.getPosY()] = from;

        activeTile = from;
    }

    @Override
    public void paintComponent(Graphics g)
    {
        super.paintComponent(g);
        setBackground(Color.BLACK);

        for (int i = 0; i < tiles.length; i++)
        {
            for(int j = 0; j < tiles[0].length; j++)
            {
                tiles[i][j].render(g, tileOffsetX, tileOffsetY);
            }
        }

        g.setColor(Color.RED);
        g.drawRect(activeTile.getPosX() * tileWidth + tileOffsetX, activeTile.getPosY() * tileHeight + tileOffsetY, tileWidth, tileHeight);
    }

    public void setWindowSize(Dimension windowSize)
    {
        this.windowSize = windowSize;
        tileOffsetX = (int) (windowSize.getWidth() - image.getWidth()) / 2;
        tileOffsetY = (int) (windowSize.getHeight() - image.getHeight()) / 2;
    }
}

Also, I'll throw in the render method from the Tile Class:

public void render(Graphics g, int offsetX, int offsetY)
    {
        g.drawImage(image, posX * tileWidth + offsetX, posY * tileHeight + offsetY, null);
    }

As you can see, I store the positions in the tile objects.

[JamesCherrill]

There's something deeply suspicious about the data redundancy here that makes me unsurprised that your swap method has problems.

As I understand it theres a Tile[][] array that says what Tile is in what position, and each Tile has its x,y position as instance variables. Change one and you must be certain to change the other. One small code error and they will get out of step, with chaotic results.

In your swap routine it looks like there are 4 Tiles:
tiles[from.getPosX()][from.getPosY()], tiles[to.getPosX()][to.getPosY()], from, and to, but in reality there are only 2 and you are swapping around their references as well as their data. I tried to follow what was really happening in your swap, but between the redundant references and the redundant data it just made my brain hurt.

Now the only reason ever to duplicate info like that is to fix a performance problem. I know you are a fan of premature optimisation, but you are paying a high cost here. Your attampt to fix a non-existant performance issue is resulting in code that you can't write correctly or fix.

IF this was my code I would be thinking about getting rid of those instance variables, and just keeping the array. (Based on a guess as to how often you need to know which Tile is in a given position, as opposed to where a given Tile is. If I'm wrong the keep the instance variables and can the array.) IF there are cases where you just have a reference to a Tile without it's x,y context and you need to know its position, then you can use public getX() and getY() methods in the Tile class that search the array to return the current answer. At this point you are probably thinking "but that's horribly inefficient". I would say "it may add 50 nanoSeconds times 1000 calls every time the user makes a move. Get real".

Bottom line. Get rid of the data redundancy and your code will immediately become shorter, simpler, and safer. (Including your swap routine, which will become trivial.)

[Doogledude123]

I'm not really following, I'm really not doing it the way I am for any optimization reason, it's really only because that's the way that makes sense to me. Since there's clearly a better way, would you be able to point me in that direction?

I don't really see what the problem is with passing in the tiles to get the position's of the active tile and the tile that should be moved. Now that I think about it though, I could just pass the positions as ints though, but what difference would that make?

[mKorbel]

1. override getPrefferedSize instead of hunting for pixels perfections e.g. diff.getWidth()/getHeight();

2. then inintial calculations about Width/Height is missunderstaning, thats should be part of paintComponent where getWidth()/getHeight() returns real pixels perfections at runtime

3. KeyListener is last of property for animations in Swing, use KeyBindings instead

4. there isn't reason to swap, anything, whatever, never, it should be part of paintComponent again, swap with content in the concrete Tile, never to moving with (you missing concept with JComponents ZOrder)

5. revalidate should be always with repaint (standard notifiers for LayoutManager)

6. don't get idea for why reason is there public void setWindowSize(Dimension windowSize), override getPrefferedSize for this container, the LayoutManager do this job perfectly by itsefl

7. code is wrong designed with lots of effort for basics optimalizations (if something will be based on this code in the future)

[JamesCherrill]

I'm not sure I can be a lot clearer than my previous post without writinga lot of sample code - and frankly I don't have that much time. The key thought is "don't hold the same information in two different places or formats at the same time".