C++ Is friend a good idea for this scenario?

Question

I have the following 2 classes declared in a C++ header:

class DrawableChunk {
    friend class Drawable;
private:
    std::queue<uint> freePos;
    std::set <Drawable*, DrawableChunkCompare> drawables;
    uint ID;
    GLuint VAO;
    GLuint VBO;
    GLuint EBO;
    long long GetMinDepth();
    long long GetMaxDepth();
    static uint globalID;
    static std::unordered_map<uint, DrawableChunk*> mapChunk;
    static std::vector<DrawableChunk*> vecChunk;

    static uint Create();
    static void InsertDrawableAndPopQueue(Drawable* drawable, DrawableChunk* chunk, std::queue<Drawable*>& queue);
    static Drawable* InsertDrawableAndEvictDMax(Drawable* drawable, DrawableChunk* chunk, std::queue<Drawable*>& queue);
    static uint GetNumChunks();
    static void AddDrawable(Drawable* drawable);
    static void RemoveDrawable(Drawable* drawable);
    static void AddDrawableData(Drawable* drawable);
public:
    static void PrintMap();
    static GLuint GetVAO(uint _id);
};

class Drawable {
    friend class DrawableChunk;
public:
    static const uint chunkSize{ 65536 };
    static const uint vertexDataSize{ 80 };
    static const uint elementDataSize{ 6 };
private:
    long long depthID;
    ulong ID;
    GLfloat x;
    GLfloat y;
    GLfloat xOrigin;
    GLfloat yOrigin;
    GLfloat angle;
    GLfloat xScale;
    GLfloat yScale;
    GLuint shader;
    GLuint color;
    uint chunkID;
    uint chunkPos;
    int depth;
    Sprite* sprite;
    uint subImage;
    bool update;
    static ulong globalID;

    static std::vector<Drawable*> vertexUpdateContainer;
    static std::unordered_map<Drawable*, std::array<GLubyte, vertexDataSize>> mapVertexData;

    static long long CalcDepthID(Drawable* drawable);
    void CheckAndAddToVertexUpdateContainer();
    void UpdateVertices();
    std::array<GLubyte, vertexDataSize>& GetVertexData();
public:

    Drawable(Sprite* _sprite, GLfloat _x, GLfloat _y, GLfloat _xOrigin, GLfloat _yOrigin, GLfloat _angle, GLfloat _xScale, GLfloat _yScale, GLuint _color, int _depth, GLuint _shader);

    static Drawable* Create(Sprite* _sprite, GLfloat _x, GLfloat _y, int _depth);
    static void UpdateVerticesForAll();
    static void CleanVertexUpdateContainer();

    Pointf GetPosition();
    Pointf GetOrigin();
    Pointf GetScale();
    ulong GetID();
    GLfloat GetAngle();
    GLuint GetColor();
    GLuint GetShader();
    uint GetChunkID();
    uint GetChunkPos();
    uint GetSubImage();
    int GetDepth();
    Sprite* GetSprite();
    long long GetDepthID();
    
    void SetSubImage(uint _subimage);
    void SetPosition(float _x, float _y);
    void SetOrigin(float _xOrig, float _yOrig);
    void SetScale(float _xScale, float _yScale);
    void SetAngle(float _angle);
    void SetColor(uint _color);
    void SetDepth(int _depth);
};

As you can see, I made each other friends, The reasoning behind this is this: These two classes depend on each other: Creating a drawable WILL put it into a chunk and Adding a drawable to a chunk WILL change the chunk's status but may also change the status of other Drawables. Without friending, I would be forced to expose several methods as public, but these methods are really called in very specific circumstances and are of no interest nor should be used outside their very specific purpose.

This has given me quite the headache the last few days. I keep reading that friending classes is very bad practice and stems from poor design. However these are really two separate entities which complement each other. I could possibly "liquidate" DrawableChunk and insert everything into Drawable but is it really worth it? These classes are really, really tightly coupled.

I have read topics like When should you use 'friend' in C++? , I am just not sure my specific design falls into the category of "you should probably use friend" or "you got it all wrong".

So with a given that right now a Drawable may change a Chunk's status and vice versa, how would you rearrange this problem? Would you leave them being friends of each other? What are your suggestions?

EDIT: Providing further clarifications. When a Drawable is created, it is added to a DrawableChunk (Chunk for simplicity). Each Drawable must track which Chunk it belongs to and at which position. So when a Drawable is added to a chunk, it chunkID and chunkPos members must be updated.

The DrawableChunk::AddDrawable(Drawable* drawable) function performs a test to see which Chunk a Drawable fits in or, if needed, a new Chunk is created. When the suitable Chunk ID and position is found, the Drawable pointer is inserted into that Chunk's container (a set) AND the Drawables chunkID and chunkPos must be updated according to that insertion.

EDIT2: I stripped away all OpenGL functionality and just left the relevant parts. This code should compile and output info on Chunks and its contents.

graphics.h

#pragma once

#include <vector>
#include <unordered_map>
#include <string>
#include <queue>
#include <set>

typedef unsigned int uint;
typedef unsigned short ushort;
typedef unsigned long ulong;
typedef unsigned long long ulonglong;

class Drawable;

struct DrawableChunkCompare {
    bool operator()(Drawable* const lhs, Drawable* const rhs) const;
};

class DrawableChunk {
    friend class Drawable;
private:
    std::queue<uint> freePos;
    std::set <Drawable*, DrawableChunkCompare> drawables;
    uint ID;
    long long GetMinDepth();
    long long GetMaxDepth();
    static uint globalID;
    static std::unordered_map<uint, DrawableChunk*> mapChunk;
    static std::vector<DrawableChunk*> vecChunk;

    static uint Create();
    static void InsertDrawableAndPopQueue(Drawable* drawable, DrawableChunk* chunk, std::queue<Drawable*>& queue);
    static Drawable* InsertDrawableAndEvictDMax(Drawable* drawable, DrawableChunk* chunk, std::queue<Drawable*>& queue);
    static uint GetNumChunks();
    static void AddDrawable(Drawable* drawable);
    static void RemoveDrawable(Drawable* drawable);
    static void AddDrawableData(Drawable* drawable);
public:
    static void PrintMap();
};

class Drawable {
    friend class DrawableChunk;
public:
    static const uint chunkSize{ 16 };

private:
    long long depthID;
    ulong ID;
    uint chunkID;
    uint chunkPos;
    int depth;
    static ulong globalID;
    static long long CalcDepthID(Drawable* drawable);
public:

    static Drawable* Create(float _x, float _y, int _depth);

    uint GetChunkID();
    uint GetChunkPos();
    long long GetDepthID();
    void SetDepth(int _depth);
};

long long SgnLLNZ(long long num);

graphics.cpp

#include "graphics.h"
#include "math_aux.h"

#include <iostream>


typedef unsigned int uint;
typedef unsigned char uchar;
typedef unsigned short ushort;
typedef unsigned long long ulonglong;

std::unordered_map<uint, DrawableChunk*> DrawableChunk::mapChunk{};
std::vector<DrawableChunk*> DrawableChunk::vecChunk{};

uint DrawableChunk::globalID{ 0 };
ulong Drawable::globalID{ 0 };

// DrawableChunk ======================================================

uint DrawableChunk::Create() {
    DrawableChunk* ch{ new DrawableChunk };
    ch->ID = ++globalID;

    //Creating various OpenGL objects..

    for (uint i{ 0 }; i < Drawable::chunkSize; ++i) {
        ch->freePos.push(i);
    }

    mapChunk.emplace(globalID, ch);
    vecChunk.push_back(ch);
    return globalID;
}

void DrawableChunk::AddDrawable(Drawable* drawable) {
    std::queue<Drawable*> queue{};
    queue.push(drawable);

    std::vector<DrawableChunk*> chunksWithFreeSpots{};
    if (vecChunk.size() > 0) {
        for (auto& it : vecChunk) {
            DrawableChunk* ch{ it };
            if (ch->freePos.size() > 0) {
                chunksWithFreeSpots.push_back(ch);
            }
        }
    }
    int phase{ 0 };

    DrawableChunk* chunkA{ nullptr };
    DrawableChunk* chunkB{ nullptr };

    while (queue.size() > 0) {
        //std::cout << queue.front() << ", " << phase << std::endl;

        Drawable* dr{ queue.front() };

        switch (phase) {
        case 0: //Any chunks at all?
        {
            if (mapChunk.size() == 0) {
                DrawableChunk* ch{ mapChunk[Create()] };
                InsertDrawableAndPopQueue(dr, ch, queue);
            }
            else {
                phase = 1;
            }
        }
        break;
        case 1: //Any free spots?
        {
            if (chunksWithFreeSpots.size() > 0) {
                phase = 2;
            }
            else {
                phase = 3;
            }
        }
        break;
        case 2: //dr fits in any chunk's empty spot OR only one chunk?
        {
            DrawableChunk* ch{ nullptr };
            long long drDepth{ dr->depthID };
            for (auto& it : chunksWithFreeSpots) {
                if (vecChunk.size() == 1 || (drDepth >= it->GetMinDepth() && drDepth <= it->GetMaxDepth())) {
                    ch = it;
                    break;
                }
            }
            if (ch != nullptr) {
                InsertDrawableAndPopQueue(dr, ch, queue);
            }
            else {
                phase = 3;
            }
        }
        break;
        case 3: //dr fits within any chunk's range?
        {
            DrawableChunk* ch{ nullptr };
            long long drDepth{ dr->depthID };
            for (auto it : vecChunk) {
                if (drDepth >= it->GetMinDepth() && drDepth < it->GetMaxDepth()) {
                    ch = it;
                    break;
                }
            }
            if (ch != nullptr) {
                queue.push(InsertDrawableAndEvictDMax(dr, ch, queue));
                phase = 0;
            }
            else {
                phase = 4;
            }
        }
        break;
        case 4: //D fits between two chunks A and B? (check from back to front)
        {
            if (vecChunk.size() > 1) {
                chunkA = nullptr;
                chunkB = nullptr;
                long long drDepth{ dr->depthID };
                for (long long i{ static_cast<long long>(vecChunk.size()) - 2 }; i >= 0; --i) {
                    if (drDepth >= vecChunk[i]->GetMaxDepth() && drDepth <= vecChunk[i + 1]->GetMinDepth()) {
                        chunkA = vecChunk[i];
                        chunkB = vecChunk[i + 1];
                        break;
                    }
                }
                if (chunkA != nullptr) {
                    phase = 5;
                }
                else {
                    phase = 6;
                }
            }
            else {
                phase = 6;
            }
        }
        break;
        case 5: //A has a free spot?
        {
            if (chunkA->freePos.size() > 0) {
                InsertDrawableAndPopQueue(dr, chunkA, queue);
            }
            else { //B has a free spot?
                if (chunkB->freePos.size() > 0) {
                    InsertDrawableAndPopQueue(dr, chunkB, queue);
                }
                else {
                    if (chunkB->GetMaxDepth() == dr->GetDepthID()) {
                        DrawableChunk* ch{ mapChunk[Create()] };
                        InsertDrawableAndPopQueue(dr, ch, queue);
                    }
                    else {
                        queue.push(InsertDrawableAndEvictDMax(dr, chunkB, queue));
                        phase = 0;
                    }
                }
            }
        }
        break;
        case 6: //dr depth < First chunks min depth?
        {
            DrawableChunk* chFirst{ vecChunk[0] };
            if (dr->GetDepthID() < chFirst->GetMinDepth()) {
                if (chFirst->freePos.size() > 0) {
                    InsertDrawableAndPopQueue(dr, chFirst, queue);
                }
                else {
                    queue.push(InsertDrawableAndEvictDMax(dr, chFirst, queue));
                    phase = 0;
                }
            }
            else {
                DrawableChunk* chLast{ vecChunk[vecChunk.size() - 1] };
                if (chLast->freePos.size() > 0) {
                    InsertDrawableAndPopQueue(dr, chLast, queue);
                }
                else {
                    DrawableChunk* ch{ mapChunk[Create()] };
                    InsertDrawableAndPopQueue(dr, ch, queue);
                }
            }
        }
        break;
        }


    }

    //DrawableChunk::PrintMap();
}

void DrawableChunk::AddDrawableData(Drawable* drawable) {
    DrawableChunk* ch{ mapChunk[drawable->chunkID] };
    uint pos{ drawable->GetChunkPos() };
}

void DrawableChunk::RemoveDrawable(Drawable* drawable) {
    DrawableChunk* ch{ mapChunk[drawable->chunkID] };
    auto it{ ch->drawables.find(drawable) };
    ch->drawables.erase(it);
    ch->freePos.push(drawable->GetChunkPos());
}

long long Drawable::CalcDepthID(Drawable* drawable) {
    long long Ldepth{ (long long)(drawable->depth) };
    long long result{ (Ldepth << 44) + ((long long)(SgnLLNZ(Ldepth)) * drawable->ID) };
    return result;
}

long long DrawableChunk::GetMinDepth() {
    auto it{ drawables.begin() };
    if (it != drawables.end()) {
        return (*it)->GetDepthID();
    } else {
        return std::numeric_limits<long long>::min();
    }
}

long long DrawableChunk::GetMaxDepth() {
    auto it{ drawables.rbegin() };
    if (it != drawables.rend()) {
        return (*it)->GetDepthID();
    } else {
        return std::numeric_limits<long long>::max();
    }
}

bool DrawableChunkCompare::operator()(Drawable* lhs, Drawable* rhs) const {
    return (lhs->GetDepthID() < rhs->GetDepthID());
}

uint DrawableChunk::GetNumChunks() {
    return static_cast<uint>(mapChunk.size());
}

Drawable* DrawableChunk::InsertDrawableAndEvictDMax(Drawable* drawable, DrawableChunk* chunk, std::queue<Drawable*>& queue) {
    auto it{ chunk->drawables.rbegin() };
    Drawable* evicted{ *it };
    chunk->drawables.erase((++it).base());
    drawable->chunkID=chunk->ID;
    drawable->chunkPos=evicted->chunkPos;
    chunk->drawables.insert(drawable);
    AddDrawableData(drawable);
    queue.pop();
    return evicted;
}

void DrawableChunk::InsertDrawableAndPopQueue(Drawable* drawable, DrawableChunk* chunk, std::queue<Drawable*>& queue) {
    drawable->chunkID=chunk->ID;
    drawable->chunkPos=chunk->freePos.front();
    chunk->freePos.pop();
    chunk->drawables.insert(drawable);
    AddDrawableData(drawable);
    queue.pop();
}

void DrawableChunk::PrintMap() {
    for (auto& it : mapChunk) {
        std::cout << "Chunk ID: " << it.first << " free spots: ";
        std::queue<uint> f{ it.second->freePos };
        std::string str{ "" };
        while (f.size() > 0) {
            str += std::to_string(f.front())+",";
            f.pop();
        }
        std::cout << str << std::endl;
        DrawableChunk* ch{ it.second };
        for (auto it2 : ch->drawables) {
            std::cout << "(ID "<<it2->ID<<" ["<<it2<<"] D "<<it2->GetDepthID() <<" "<<"pos:"<<it2->chunkPos<< ") ";
        }
        std::cout << std::endl;
    }
}

Drawable* Drawable::Create(float _x, float _y, int _depth) {
    Drawable* dr{ new Drawable() };
    dr->ID=++globalID;
    dr->depth = _depth;
    dr->depthID = CalcDepthID(dr);
    DrawableChunk::AddDrawable(dr);
    return dr;
}

// Drawable ======================================================


void Drawable::SetDepth(int _depth) {
    DrawableChunk::RemoveDrawable(this);
    depth = _depth;
    depthID = CalcDepthID(this);
    DrawableChunk::AddDrawable(this);
}

uint Drawable::GetChunkID() {
    return chunkID;
}
uint Drawable::GetChunkPos() {
    return chunkPos;
}

long long Drawable::GetDepthID() {
    return depthID;
}

long long SgnLLNZ(long long num) {
    return +1 | (num >> (sizeof(long long) * 8 - 1));
}

main.cpp

#include "graphics.h"

#include <iostream>
#include <vector>

typedef unsigned int uint;
typedef unsigned char uchar;

std::vector<Drawable*> drawableContainer{};

int main()
{

    for (uint i{ 0 }; i < 24; ++i) {
        int r{ rand() % 500 };
        Drawable* e1 {Drawable::Create((float)((rand() % 800)), (float)((rand() % 600)), r)};
        drawableContainer.push_back(e1);
    }

    DrawableChunk::PrintMap();

    
    return 0;
}

Answer 1

No it aint, friend is more for implementing in a top level, for simple tasks the parent-child is the ideal. How to implement it is as simple as describing what it needs to do "A structure that handles a list of shaders".

All of the "not good" parts in your code can be fixt by using the tools that c++ has and not complicating parts that don't need to be.

Adding a quick refactor to the fist price of code adding the use of stdint.h and removing the use of private makes it look better but not perfect. The code is more to give an idea than to replace yours.

// This header is included 
// in all std libraries and
// defines all uint32_t like
// variables types
#include <cstdint>

class DrawableChunk {

    public:

        bool update;
        Sprite * sprite;

        int32_t depth, subImage;
        uint32_t chunkID, chunkPos;
        uint64_t ID, depthID, globalID;

        GLuint shader, color;
        GLfloat x, y, xOrigin, yOrigin;
        GLfloat angle, xScale, yScale;

        uint32_t chunkSize{ UINT16_MAX };
        uint32_t vertexDataSize{ 80 };
        uint32_t elementDataSize{ 6 };

        DrawableChunk(Sprite * _sprite, GLfloat _x, GLfloat _y, 
                      GLfloat _xOrigin, GLfloat _yOrigin, 
                      GLfloat _angle, GLfloat _xScale, GLfloat _yScale, 
                      GLuint _color, int32_t _depth, GLuint _shader);

        // ~DrawableChunk(); // missing (?)
        // DrawableChunk::Create() conteins a new but i see no delete
        DrawableChunk * Create(Sprite* _sprite, GLfloat _x, GLfloat _y, int32_t _depth);

        int64_t CalcDepthID(Drawable* drawable);
        void UpdateVertices();
        void UpdateVerticesForAll();
        void CleanVertexUpdateContainer();
        void CheckAndAddToVertexUpdateContainer();

};


class Drawable {

    public:

        std::queue<uint32_t> freePos;
        std::vector<DrawableChunk *> vecChunk;

        uint32_t ID, globalID;
        GLuint VAO, VBO, EBO;

        int64_t GetMinDepth();
        int64_t GetMaxDepth();

        // can be replaced by std::vector functions
        uint32_t GetNumChunks();
        uint32_t Create();
        uint32_t create_chunk(Sprite* _sprite, GLfloat _x, GLfloat _y, int32_t _depth); // gives the arguments to the child after checks
        
        std::unordered_map<uint32_t, DrawableChunk*> mapChunk;
        std::set <DrawableChunk*, DrawableChunkCompare> drawables;

        void PrintMap();
        void AddDrawable();
        void RemoveDrawable();
        void AddDrawableData();

        // Drawable* drawable / std::queue<Drawable*>& queue, is already accesible
        void InsertDrawableAndPopQueue(DrawableChunk* chunk);
        DrawableChunk * InsertDrawableAndEvictDMax(DrawableChunk* chunk);

};