1

I am using std::unique_ptr<uint8[]> CPPPixelBuffer; to store pixel data of a texture as an array.

This array is initialized in the constructor as followed:

SIZE_T BufferSize = WorldTextureWidth * WorldTextureHeight * DYNAMIC_TEXTURE_BYTES_PER_PIXEL;
CPPPixelBuffer = std::make_unique<uint8[]>(BufferSize);

The creation and drawing of the texture is working so far. (as shown on the picture below) TextureData as the are supposed to be

Now I am trying to create a copy of that array using a for loop. (I am using a for loop because I want to extract just parts of the texture later on. But just for demonstration I copy the array completly in this example.)

SIZE_T PartBufferSize = WorldTextureWidth * WorldTextureHeight * DYNAMIC_TEXTURE_BYTES_PER_PIXEL;
std::shared_ptr<uint8[]> PartPixelBuffer(new uint8[PartBufferSize]());

// Get the base pointer of the pixel buffer
uint8* Ptr = CPPPixelBuffer.get();

//Get the base pointer to the new pixel buffer
uint8* PartPtr = PartPixelBuffer.get();

for (int i = 0; i < WorldTextureHeight *WorldTextureWidth * DYNAMIC_TEXTURE_BYTES_PER_PIXEL; i++) {

        *(PartPtr++) = *(Ptr++);
}

delete Ptr;
delete PartPtr;

The pixels after copying are mixed up and the picture is different every time I execute this code. (as shown on the picture below) Wrong Reults

What am I doing wrong?

Schrello
  • 27
  • 5
  • 4
    You don't explicitly delete pointers that are owned by smart pointers – The Dreams Wind Dec 16 '22 at 12:20
  • 2
    On the other hand, a mandatory question: why are you not using `std::vector`? – Passer By Dec 16 '22 at 12:24
  • I am thinking it would be more efficient to use an array with fixed size than a dynamic array (or vector). – Schrello Dec 16 '22 at 12:30
  • 2
    why do you think so ? Maybe it is faster, but before you profiled and measured, this is really just [cargo cult](https://en.wikipedia.org/wiki/Cargo_cult_programming) – 463035818_is_not_an_ai Dec 16 '22 at 12:32
  • 1
    *'I am thinking it would be more efficient'*, so much bad code starts with this thought. Write the code the simple way first, then see if you have an efficiency problem, very often you won't, but if you do, then improve the code you have already written. – john Dec 16 '22 at 12:37
  • This code *screams* that it should be using `std::vector`. – Eljay Dec 16 '22 at 12:38
  • 1
    BTW you don't need to deal with raw data here at all. The smart pointers support subscript operators just fine: `PartPixelBuffer[i] = CPPPixelBuffer[i];` – The Dreams Wind Dec 16 '22 at 12:39
  • Thank you... I have just tried `PartPixelBuffer[i] = CPPPixelBuffer[i];` but the behavior remains the same :( – Schrello Dec 16 '22 at 12:52
  • Not using `std::vector` would be (marginally) faster *if you didn't perform dynamic allocation anyway!* As it stands, you are emulating a vector which makes little sense ;-). – Peter - Reinstate Monica Dec 16 '22 at 12:54
  • It also feels wrong to first create smart pointers and then extract the raw pointers for operations *that you implement yourself.* (I think the main reason get() exists at all is to serve cases like *existing* library functions which are old and don't know better than to take raw pointers.) If you need to pass the pointer around in your own code, use shared pointers. – Peter - Reinstate Monica Dec 16 '22 at 13:01
  • Yeah. Thank you guys. I am going to try it with a vector. Just to explain: I am using Java and Python for 10 years now and want to learn a new language. Thats also one reason I tried the smartpointer. Vectors are just as in Java ^^' – Schrello Dec 16 '22 at 13:03
  • But just for the record: It should work, shouldn't it? – Schrello Dec 16 '22 at 13:04
  • knowing java and python is not a big advantage when you want to learn C++. You basically have to unlearn everything. Among those three Java and python have some commonalities, while C++ is completely different. Don't get fooled by similar looking syntax – 463035818_is_not_an_ai Dec 16 '22 at 13:08
  • in the code you just added in the last loop you increment the pointer twice in each iteration. Already in the second iteration you are out of bounds. Please do not alter the question to ask about something else or about an error in different code after you received answers already – 463035818_is_not_an_ai Dec 16 '22 at 13:13
  • if you remove the addtional increment you get expected output : https://godbolt.org/z/cs9rhbYKo – 463035818_is_not_an_ai Dec 16 '22 at 13:16
  • and you can get the same without any manual pointer arithmetic https://godbolt.org/z/Mdvo3oYTh – 463035818_is_not_an_ai Dec 16 '22 at 13:17
  • Ah...yes... you are right. That does explain this. i am going to delet this edit than. – Schrello Dec 16 '22 at 13:17
  • what you're doing wrong is deleting the array and the copy. – user253751 Dec 16 '22 at 14:03

2 Answers2

2

The last two lines are triple wrong.

  1. the arrays are managed by smart pointers. You should not delete them manually.
  2. the arrays are created via new[]. Freeing the memory would need delete[]
  3. the pointers are not pointing to the beginning of the array, because they have been incremented in the loop

Each point alone would cause undefined behavior.

You should remove the last two lines. It is unclear why you think you would need to detele the original and the copy after copying the data. The copying itself looks ok (though there are simpler way to copy the array as mentioned in comments).

463035818_is_not_an_ai
  • 109,796
  • 11
  • 89
  • 185
-1

Firstly, you use arrays that managed by smartpointers. This means, that you don't need to call delete to free memory, these pointers will be freed when they will be out-of-scope. Secondly, while coding in C++, use std::vector instead of raw arrays, because it has some advantages like better safety. Thirdly, if you use smartpointers, then do not allocate memory explicitly, use functions that will do that for you. In your case, you might want to use std::make_shared<>(). This is example of code, where I create one smartpointer to std::vector, that stores uint8_t, and after that, I copy that pointer to another pointer:

#include <iostream>
#include <memory>
#include <vector>

int main(int argc, char* argv[])
{

    auto ptr1 = std::make_shared<std::vector<uint8_t>>();
    auto ptr2 = ptr1;

    std::cout << ptr1 << '\n';
    std::cout << ptr2 << '\n';

    return 0;
}

This will print addresses of memory which smartpointers are pointing to. Note that every run of program you will see different memory addresses, but these two addresses will be the same. If you want to copy array when using smartpointers, you can do this trick:

#include <iostream>
#include <memory>
#include <vector>

int main(int argc, char* argv[])
{

    auto ptr1 = std::make_shared<std::vector<uint8_t>>();
    std::vector<uint8_t> vec = *ptr1;

    std::cout << ptr1 << '\n';
    std::cout << &vec << '\n';

    

    return 0;
}

This trick will let you to copy array. When running that code, array which is pointed by ptr1 and vec will be equal, but with different memory addresses.

Bolderaysky
  • 268
  • 1
  • 8