Return to Revisions

I'm not sure I understand your code entirely. Say for example I have no idea what complexArray is. I was about to shy away from writing a review because of that. But thinking about it, not understanding code might be the best reason to review it.

but the complication in simplifying this is in the fact that adding or removing tiles to the bottom and top is implicitly different than adding them to the sides because in the former I'm adding and removing whole arrays in a 2D array, and in the latter, I'm removing or adding just one element in the 2nd level array.

And this leads to the question why you add and remove elements this way. I cannot think of a reason. You have a lot of code that is busy taking elements out of some 2D array and putting it into another.

// add tile from arrGround to onScreeArray
onScreenArray[topRow].push(t);
// remove tile from arrGround to onScreenArray
arrGround[tType].pop();

Why? Especially the part the places elements back into the ground array is not plausible. Why put things back at all? Why not just leave them in the ground array? It looks like you modelled your data structures so that they are mutually exclusive: a Tile is either on screen or on the "ground" which. It's never both. I think this is where the problem is at, because it appears to be an unnecessary overcomplication.

You are dealing with Objects which are referenced. Having several arrays that each reference an object is not much of an overhead. It's not like you are duplicating data to fill up memory.

think top down, not bottom up

You implemented the functionality to move in all 4 directions. That's good, because you can combine those to create any other more complex motion, right? Right. But it's somewhat clumsy. If you want to go 5 tiles to the right or to some arbitrary position, your code doesn't work too well.

Instead, approach the problem more abstractly. The general problem you are trying to solve is this:

• You have some 2D array which stores all tile objects. This is probably what arrGround is in your current code. (again, I'm not 100% sure how your code works, so I might be wrong on that) This array is constant. For my answer, I will call this the Map
• There's a number of Tile objects that you want to display. The number of them is constant and determined by the width and height of what you use for display. This is probably what onScreenArray is in your code. This is always a subset of Map.
• You want to be able to change what Tiles are in the aforementioned subset, so that you can display other parts of the Map.

The above boils down to something usually called a camera.

public class Map
{
    private const tileGrid:Array = [];
    private const camera:Rectangle = new Rectangle(0, 0, 10, 7); // display 10 by 7 tiles
    
    public function Map()
    {
        // build 2D array tileGrid here from whatever
    }
    
    public function setCameraPosition(x:Number, y:Number):void
    {
        camera.x = x;
        camera.y = y;
    }
    
    public function copySubset(target:Array):Array
    {
        // use camera object as boundaries to decide what's part of the subset and what's not
        
        for(int x:int = camera.left; x < camera.right; ++x)
            for(int y:int = camera.top; y < camera.bottom; ++y)
                target[x][y] = tileGrid[x][y];
    }
}

Treat the above as pseudo code as I haven't tested it. As you can see, the idea is to have some Rectangle object that acts as a camera that defines some subset of the entire map.

You can now pass your onScreen 2D array of appropriate size to the copySubset() method and it will be filled with whatever is visible by the camera. When you want to pan around, just pass other variables to setCameraPosition(), then copy again.

This code is far from complete. No values are sanity checked. This should be several classes Camera, Map, etc. You could build connections via observer pattern (EventDispatcher) to update propagate changes of values throughout all objects interested in them automatically. But the basic idea should be clear.

tl, dr;

The problem of your existing code is that you have manoeuvred yourself into some niche of use cases. On first sight, it looks like a good idea to build the specific cases, but by doing that it's overlooked that the general solution is far simpler.

edit

complexArray is the array of index numbers that tell us where from the tile map bitmap to draw the bitmap data (i.e., 1 is ground... But what type of ground is dependent on what is surrounding it, so the complex array may turn a 1 (ground) into a 13, which is a cliff edge)

This functionality should executed before your code runs, likely even before the program is compiled.

Don't just leave a hint on every Tile object how to find some clue in some other data structure how to render it. Figure that out as soon as possible.

• If the map is fixed and never changes during execution, the status of each tile should be calculated prior to compilation. Create some kind of map editor or parser or whatever program to calculate the exact type of each Tile. Run this program before you hit compile.
• If the map changes at runtime, you'd have to include that logic into your program. Execute it when the map changes and only then.

The problem with your existing code is that it distributes data of Tile objects across several data structures. That's bad. You have to fiddle around with all those arrays just to look up some number that might have a fixed value that was know even before the program existed. There's no need to look it up across several arrays every time you try to render a tile.

As your code merely deals with deciding what's displayed and what's not, this shouldn't be of interest at all. Maybe there's a property .type in the Tile class that holds a value to represent its type. Maybe this value has to be recalculated because the map changes. But the code you posted is not the place to do that.