-1

I'm coding a game for my class, and I'm having a segmentation fault when my map array tries to access the symbol of a piece of land to display on a map. I can't figure out how to word this, because I can't figure out where to start when fixing this issue. Any help is appreciated.

Map.h

    public:
        Map(Player &player) {
            ...
            //Create Map Array Using Size Parameters
            Land *map[map_X][map_Y];
            this->map = **↦

            //Generate Lands on Map
            BuildMap(player);
        }
    ...
    private:
        Land **map;
        ...
    }
};

Map.cpp

void Map::BuildMap(Player &player) {
    ...
    for(size_t i = 0; i < map_Y; i++) {
        for(size_t k = 0; k < map_X; k++) {
        
            //!TEST iteration; REMOVE
            std::cout << "BuildMap [" << k << "][" << i << "]" << std::endl;

            map[k][i] = *GetRandomLand();
        
            //!TEST land; REMOVE
            std::cout << "Symbol: \'" << map[k][i].GetSymbol() << "\'" << std::endl
                      << std::endl;

            //!TEST failsafe; REMOVE
            if(k > 100) {
                break;
            }
        }
    }
}

Land.cpp

Land* GetRandomLand() {
    srand(time(NULL));
    LandTypes selection = (LandTypes)(rand() % MAX_LAND_TYPES);

    switch(selection) {
        case LAKE:
            return new Lake;
            break;
        case FOREST:
            return new Forest;
            break;
        case DESERT:
            return new Desert;
            break;
        default;
            return new Forest;
            break;
    }
}

Edit: I forgot to show it here, but map is a Land pointer. Specifically, Land **map.

Herodegon
  • 41
  • 5
  • If you're subclassing things then you need pointers, smashing it into a fixed `map` isn't going to work. Consider wrapping in `unique_ptr`. Virtual functions require pointers. – tadman Apr 19 '21 at 23:56
  • Remember, for every `new` you **must** have a corresponding `delete`. This code leaks memory, you completely disregard the allocation in `GetRandomLand()`. The easy fix is to delegate that responsibility to a container like `unique_ptr`. – tadman Apr 19 '21 at 23:57
  • @tadman I didn't communicate it in the post, but ```map``` is a pointer array used to store derived classes of Land. – Herodegon Apr 20 '21 at 00:06
  • Please show the definition as that is *very* important. – tadman Apr 20 '21 at 00:06
  • @tadman Sorry for the confusion. I've gone and added the extra information into the post. – Herodegon Apr 20 '21 at 00:13
  • Is `map_X` and ``map_Y` constant? or dynamic value? As other state, change the to return `std::unique` from `GetRandomLand()` method. Use `std::array` (if constant), or `std::vector` (if dynamic) to replace your member variable `map`. – Vuwox Apr 20 '21 at 00:19
  • I'm running c++11 for my project. I didn't realize how big of a difference that made. Leave it to me to leave out another crucial piece of information lol. – Herodegon Apr 20 '21 at 01:04
  • Tip: Use a 1D `std::vector>`. – tadman Apr 20 '21 at 01:06
  • @tadman How can I convert the derived classes I return in ```GetRandomLand()``` without ```make_unique()```? – Herodegon Apr 20 '21 at 01:11
  • You don't "convert" them, that's the point of polymorphism. – tadman Apr 20 '21 at 19:40
  • @tadman I meant I was trying to avoid using a higher version of C++, but I just went ahead an changed to C++14. – Herodegon Apr 20 '21 at 19:47
  • Using features from a C++ standard that's now 7 years old is hopefully not a huge stretch here. If you can, use C++20. There's a lot of very convenient things in it. – tadman Apr 20 '21 at 19:51
  • 1
    @tadman My professor wants us to use C++11 for our projects. I had to talk to them just so I could move up to C++14. – Herodegon Apr 20 '21 at 21:13
  • Sorry to hear you're in one of *those* courses that is, at best, C+. – tadman Apr 20 '21 at 21:33

2 Answers2

3

I tried to made two examples. One with size known at compile time (constant), and when dynamic. This following code is using C++ best practice of using RAII to avoid memory leakage.

class Map
{
    public:
    
        // Ctor for constant map.
        Map(Player& player);
        
        // Ctor for dynamic map.
        Map(Player& player, const std::size_t x, const std::size_t y);
    
    private:
    
        // Member private function to build the map with proper Land.
        // This will fill the 'constant' map.
        const std::size_t map_X = 10;
        const std::size_t map_Y = 10;    
        BuildMapConst(Player& player);
        
        
        // Member private function to build the map with proper Land.
        // This will fill the 'dynamic' map, which has been allocated in ctor.
        BuildMapDyn(Player& player);
    
        // If size is known at compile time, 2-dimension std::array should be the best.
        std::array<std::array<std::unique_ptr<Land>, map_Y>, map_X> m_const_map;
        
        // If size is unknown at compile time, 2-dimension dynamic container such as vector
        // is the way to go, since it will handle proper the deletion of the unique_ptrs.
        std::vector<std::vector<std::unique_ptr<Land>>> m_dyn_map;
    }
};


std::unique_ptr<Land> GetRandomLand() {
    srand(time(NULL));
    LandTypes selection = (LandTypes)(rand() % MAX_LAND_TYPES);

    switch(selection) {
        case LAKE:
            return std::make_unique<Lake>(); // note break are useless since you return.
        case FOREST:
            return std::make_unique<Forest>();
        case DESERT:
            return std::make_unique<Desert>();
        default;
            return std::make_unique<Forest>();
    }
}


Map::Map(Player &player) 
{
    // Constant map is allocated at compile time with fix size.
    BuildMapConst(player);      
}


void Map::BuildMapConst(Player& player) 
{        
    for(size_t i = 0; i < map_X; i++) {
        auto& map_ref = m_const_map[i];
        
        for(size_t k = 0; k < map_Y; k++) {                  
        
            //!TEST iteration; REMOVE
            std::cout << "BuildMap [" << i << "][" << k << "]" << std::endl;

            map_ref[k] = std::move(GetRandomLand());
        
            //!TEST land; REMOVE
            std::cout << "Symbol: \'" << map[k].GetSymbol() << "\'" << std::endl
                      << std::endl;

            //!TEST failsafe; REMOVE
            if(k > 100) {
                break;
            }
        }
    }
}


Map::Map(Player &player, const std::size_t x, const std::size_t y) 
{
    // Allocate properly the map.
    m_dyn_map.resize(x);
    for(std::size_t r = 0; r < x; ++r)
        m_dyn_map[r].resize(y);
    
    // Fill the dynamic map.
    BuildMapDyn(player);      
}

void Map::BuildMapDyn(Player& player) {
    
    const std::size_t rows = m_dyn_map.size();
    const std::size_t cols = m_dyn_map[0].size();
    
    for(size_t i = 0; i < rows; i++) {
        auto& map_ref = m_dyn_map[i];
        
        for(size_t k = 0; k < cols; k++) {      
            
        
            //!TEST iteration; REMOVE
            std::cout << "BuildMap [" << i << "][" << k << "]" << std::endl;

            map_ref[k] = std::move(GetRandomLand());
        
            //!TEST land; REMOVE
            std::cout << "Symbol: \'" << map[k].GetSymbol() << "\'" << std::endl
                      << std::endl;

            //!TEST failsafe; REMOVE
            if(k > 100) {
                break;
            }
        }
    }
}

EDIT

Here some reference for your question in the comments.

std::unique_ptr is holding the ownership of your base class Land.

std::unique_ptrdoesn't have copy-ctor, or assignment-copy ctor, it must be moved

RAII is used in order to allocate variable on stack and ensure deletion when out-of-scope.

Container element can be accessed via operator[] (same for std::array), a reference to the object element is provided. Which in our case is an std::vectorstd::unique_ptr>, for second dimension of the map.

Vuwox
  • 2,331
  • 18
  • 33
  • What's ```auto&``` and ```std::move()```? – Herodegon Apr 20 '21 at 00:47
  • For `auto&`, its because I dont want to call the container (array or vector) accessor on first dimension multiple time. So for efficiency, I cache a reference on each row in the outter loop. And as its name said `std::unique_ptr` is unique. It cannot be copied, so we need to use `std::move` in order to assign it at the position in the container. Note, I will add reference in above question. – Vuwox Apr 20 '21 at 01:04
  • Note that i missed the `[r]` accessing during the allocation of the second dimension, such as `m_dyn_map[r].resize(y)`. Please ensure to recopy code if you did. – Vuwox Apr 20 '21 at 01:14
  • If you don't mind me asking: what's the naming convention you use? – Herodegon Apr 20 '21 at 01:28
  • I normally use CamelCase for `class`, `struct` and `enum`. And prefer to use snake_case for `function` and `variable`. The C++ standard is mostly using this too. Some people prefer to use camelCase (note first c is not capital) for functions and variables. But its up to you, as long as you are consistent. – Vuwox Apr 20 '21 at 01:31
0

The issue likely lies here

          map[k][i] = *GetRandomLand();

Since we are dereferencing the Land* returned by GetRandomLand, you are falling subject to the object slicing problem. See What is object slicing?

I assume we are doing that because map is not defined as a 2D array of pointers, but rather as a 2D array of objects.

To ensure you have an array of pointers instead of an array of objects you could use the following syntax. If you are passing the map, it could be Land* map[][] or Land***

Land map[10][10];
// vs
Land* map[10][10];

As others have noted in the comments, polymorphism only works when you have a pointer or a reference to the object. If you dereference the object to the base class, then it is likely that your base class implementation of GetSymbol is causing the crash.

yano
  • 4,095
  • 3
  • 35
  • 68
  • How could I engage polymorphism in my array then? My goal is to have an pointer array so that I can store derived classes of Land, but to also give the user the freedom to determine the size of the map, and therefore access to define the 'x' and 'y' of ```map[x][y]```. – Herodegon Apr 20 '21 at 00:21
  • @Herodegon in the example I show `Land* map[][]`, so a 2D array of pointers. That's what you want. If you are not using the bracket notation it will be a triple pointer `Land***` – yano Apr 20 '21 at 00:24
  • Okay, I think I'm getting it. I can't use bracket notation because the user gets to choose the size of the array, so I can't define it beforehand. I tried converting my original map pointer from ```Land **map``` to ```Land ***map```, but now I can't find a way to attach a reference from map array ```Land *map[map_X][map_Y]``` to my private definition. – Herodegon Apr 20 '21 at 00:36
  • the member variable must also be of type `Land***` and you should just directly assign it instead of that `**&` fiasco. `this->map = map;` – yano Apr 20 '21 at 00:37
  • I... didn't know that was possible. So, when I try to do ```Land *map[map_X][map_Y]; this->map = map;``` I get an error: "cannot convert 'Land* [(Map*)this)->Map::map_X][(Map*)this)->Map::map_Y]' to 'Land***' in assignment." – Herodegon Apr 20 '21 at 00:39
  • @Herodegon sorry I was not thinking about this. Indeed direct conversion here is an error. In the C days, not so. What you will want is `this->map = reinterpret_cast(map)` HOWEVER I'm now realizing you will have more issues this way. Since the map you are constructing will go out of scope when the ctor finishes, you must allocate the member variable map on the heap. This is getting complicated and not really best practice. You should consider using nested std::vector instead. `std::vector> map;` for your member variable. – yano Apr 20 '21 at 00:49
  • Thanks for your help. Also, I just realized I left out that my project is running on C++11. Sorry :( – Herodegon Apr 20 '21 at 01:05