Bit set function not working correctly?

[Cyclone]

Good day,

I don't know how to ask this as I don't totally understand things but I'm having an issue.

My program reads tiles and tile sets from a game. They are 4bits per pixel. And are stored according to bitplanes.

This is working correctly but I came across a sprite where the top row of an 8x8 tile does not read correctly. Not sure why as other sprites are reading/writing correctly. If its the game or my code I do not know. Below is the code where the tile does not read correctly.

I think it might be a problem with the bit set code. Maybe there is a bit that is set different in the game???

ps. Sorry for all the questions! :(

[Cyclone]

#include "C:\Users\Chris\source\repos\PNG\lodepng.h"
#include "C:\Users\Chris\source\repos\PNG\lodepng.cpp"
#include <fstream>
#include <sstream>
#include <string>
#include <atlstr.h>
#include <iterator>
#include <iostream>
#include <bitset>
using namespace std;

//Encode from raw pixels to disk with a single function call
//The image argument has width * height RGBA pixels or width * height * 4 bytes
void encodeOneStep(const char* filename, std::vector<unsigned char>& image, unsigned width, unsigned height) {
    //Encode the image
    unsigned error = lodepng::encode(filename, image, width, height);

    //if there's an error, display it
    if (error) std::cout << "encoder error " << error << ": " << lodepng_error_text(error) << std::endl;
}

[Cyclone]

int main()
{
    const size_t width = 256, height = 256;
    const size_t no_elems = width * height * 4;
    using Image = std::vector<unsigned char>;
    std::vector<Image> Image_PNG(1000, Image(no_elems));

    CString File_Path;
    char sztmp[1024];
    const char* filepath = " ";

    cin.clear();
    cout << "Enter Path to extract sprite graphics to: " << endl;
    cin.getline(sztmp, sizeof(sztmp), '\n');
    filepath = sztmp;

    // Donkey Kong Pallette
    static const unsigned char coloursB[16][4] = { // An array of colour values. 
       //BBB  GGG  RRR
       { 0, 0, 0, 0 }, /* White    */
       {   24,  16,  0, 255 }, /* Index  1 */
       {   64,  32, 0, 255 }, /* Index  2 */
       {  96, 48, 0, 255 }, /* Index  3 */
       {   136, 64, 0, 255 }, /* Index  4 */
       {   168, 80, 0, 255 }, /* Index  5 */
       { 192, 96, 0, 255 }, /* Index  6 */
       { 208, 112, 0, 255 }, /* Index  7 */
       { 248, 0, 0, 255 }, /* Index  8 */
       { 128, 0, 0, 255 }, /* Index  9 */
       {  152,   48,  0, 255 }, /* Index 10 */
       {   248,   128, 112, 255 }, /* Index 11 */
       {  248,  160, 144, 255 }, /* Index 12 */
       {   248, 192, 184, 255 }, /* Index 13 */
       {  248, 224,   216, 255 }, /* Index 14 */
       {   248,   248,   248, 255 }  /* Index 15 */
    };

    // Create the Bitplanes
    int* Bitplane0__ROW = new int[10000];
    int* Bitplane1__ROW = new int[10000];
    int* Bitplane2__ROW = new int[10000];
    int* Bitplane3__ROW = new int[10000];

    long long int address = 0x381C46;  // add 0xC00000 to get the SNES address.
    int offset = 0; // The location of the next set of sprites.
    int Data_Size = 0;
    int x_coord = 0;
    int y_coord = 0;
    int X;

    char oDataK[0x10000]; // Sprite buffer
    ifstream inFileK;
    inFileK.open("C:\\Users\\Chris\\Desktop\\dkc1.sfc", ios::binary); // Note. this opens a binary stream. This is needed. cuzz the program was using 0x1a values as a Substitute AscII charter which halts the program/Using it as End Of File.
    inFileK.seekg(address + Data_Size, 1); // Seek to the current sprite header location.
    inFileK.read(oDataK, 0x10000);
    inFileK.close();

    int Header = 8;
    int tile_offset = 0;
    int Size_of_coordenants = (oDataK[0] + oDataK[1]) * 2;   // 0 contains the value of the number of 2x2 chars (16 x 16 pixel tiles). 1 contains the value of the number of 1x1 chars (8x8 pixel tiles). These two values are multiplied by 2 (two bytes for each set of coordenants).
    int Start_Of_First_Tile = Header + Size_of_coordenants;  // 8 is the size of the header. 6 is the total size of coordinates(3 chars (tiles) * 2 bytes for earch char) (these are also part of the header...) 

   // Assign each Bitplane Row the data from the buffer. oData is the buffer containing the bytes from the rom.
    for (int p = 0, t = 0; t < 5; t++) // 16x16 Tiles
    {
        p = 24;
        tile_offset = (32 * 17);

        Bitplane0__ROW[p++] = oDataK[14 + Start_Of_First_Tile + tile_offset];
        Bitplane0__ROW[p++] = oDataK[12 + Start_Of_First_Tile + tile_offset];
        Bitplane0__ROW[p++] = oDataK[10 + Start_Of_First_Tile + tile_offset];
        Bitplane0__ROW[p++] = oDataK[8 + Start_Of_First_Tile + tile_offset];
        Bitplane0__ROW[p++] = oDataK[6 + Start_Of_First_Tile + tile_offset];
        Bitplane0__ROW[p++] = oDataK[4 + Start_Of_First_Tile + tile_offset];
        Bitplane0__ROW[p++] = oDataK[2 + Start_Of_First_Tile + tile_offset];
        Bitplane0__ROW[p++] = oDataK[0 + Start_Of_First_Tile + tile_offset];

        p = 24;

        Bitplane1__ROW[p++] = oDataK[15 + Start_Of_First_Tile + tile_offset];
        Bitplane1__ROW[p++] = oDataK[13 + Start_Of_First_Tile + tile_offset];
        Bitplane1__ROW[p++] = oDataK[11 + Start_Of_First_Tile + tile_offset];
        Bitplane1__ROW[p++] = oDataK[9 + Start_Of_First_Tile + tile_offset];
        Bitplane1__ROW[p++] = oDataK[7 + Start_Of_First_Tile + tile_offset];
        Bitplane1__ROW[p++] = oDataK[5 + Start_Of_First_Tile + tile_offset];
        Bitplane1__ROW[p++] = oDataK[3 + Start_Of_First_Tile + tile_offset];
        Bitplane1__ROW[p++] = oDataK[1 + Start_Of_First_Tile + tile_offset];


        p = 24;

        Bitplane2__ROW[p++] = oDataK[30 + Start_Of_First_Tile + tile_offset];
        Bitplane2__ROW[p++] = oDataK[28 + Start_Of_First_Tile + tile_offset];
        Bitplane2__ROW[p++] = oDataK[26 + Start_Of_First_Tile + tile_offset];
        Bitplane2__ROW[p++] = oDataK[24 + Start_Of_First_Tile + tile_offset];
        Bitplane2__ROW[p++] = oDataK[22 + Start_Of_First_Tile + tile_offset];
        Bitplane2__ROW[p++] = oDataK[20 + Start_Of_First_Tile + tile_offset];
        Bitplane2__ROW[p++] = oDataK[18 + Start_Of_First_Tile + tile_offset];
        Bitplane2__ROW[p++] = oDataK[16 + Start_Of_First_Tile + tile_offset];

        p = 24;

        Bitplane3__ROW[p++] = oDataK[31 + Start_Of_First_Tile + tile_offset];
        Bitplane3__ROW[p++] = oDataK[29 + Start_Of_First_Tile + tile_offset];
        Bitplane3__ROW[p++] = oDataK[27 + Start_Of_First_Tile + tile_offset];
        Bitplane3__ROW[p++] = oDataK[25 + Start_Of_First_Tile + tile_offset];
        Bitplane3__ROW[p++] = oDataK[23 + Start_Of_First_Tile + tile_offset];
        Bitplane3__ROW[p++] = oDataK[21 + Start_Of_First_Tile + tile_offset];
        Bitplane3__ROW[p++] = oDataK[19 + Start_Of_First_Tile + tile_offset];
        Bitplane3__ROW[p++] = oDataK[17 + Start_Of_First_Tile + tile_offset];

    }

    /////////////////////////////////////////////////////////////////////////////////////////////////

    Start_Of_First_Tile = Start_Of_First_Tile + Data_Size; // the location of the curent sprite.
    int c = 0;
    for (int p = 24; p < 32; p++)
    {
        // Bottom Right of 16x16
        if (p == 24) { c = 256 * (y_coord + 15) * 4 + x_coord + 0 + 0;; }
        if (p == 25) { c = 256 * (y_coord + 14) * 4 + x_coord + 0 + 0;; }
        if (p == 26) { c = 256 * (y_coord + 13) * 4 + x_coord + 0 + 0;; }
        if (p == 27) { c = 256 * (y_coord + 12) * 4 + x_coord + 0 + 0;; }
        if (p == 28) { c = 256 * (y_coord + 11) * 4 + x_coord + 0 + 0;; }
        if (p == 29) { c = 256 * (y_coord + 10) * 4 + x_coord + 0 + 0;; }
        if (p == 30) { c = 256 * (y_coord + 9) * 4 + x_coord + 0 + 0;; }
        if (p == 31) { c = 256 * (y_coord + 8) * 4 + x_coord + 0 + 0;; }

        // Write the PNG Images
        if (p < 32)
        {
            for (int j = 0, N = 7; j < 8; j++, N--) // Check which bits are set for each pixel of the tiles. Then assign the index colour to the image Array location.
            {
                std::bitset<4> bs;
                bs.set(0, (Bitplane0__ROW[p] >> N) & 1);
                bs.set(1, (Bitplane1__ROW[p] >> N) & 1);
                bs.set(2, (Bitplane2__ROW[p] >> N) & 1);
                bs.set(3, (Bitplane3__ROW[p] >> N) & 1);
                unsigned long Index = bs.to_ulong();

                Image_PNG[X][c++] = coloursB[Index][0]; // Red
                Image_PNG[X][c++] = coloursB[Index][1]; // Green
                Image_PNG[X][c++] = coloursB[Index][2]; // Blue 
                Image_PNG[X][c++] = coloursB[Index][3]; // Alpha    
            }
        }
    }
    string combined_file_path(string(filepath) + to_string(X) + ".png");
    encodeOneStep(combined_file_path.c_str(), Image_PNG[X], width, height);  // Write the .png image file.

    return 0;
}

[DizzyDon]

The for loop for (int p = 0, t = 0; t < 5; t++) doesn't make sense.
The looping variable t is never used and nothing else changes between the iterations, so the code blocks within the loop do exactly the same thing over and over.
Also, when you find yourself numbering variables (Bitplane0, etc), it's probably time for an array.
It seems that the following would do the same as your code.

    int* Bitplane[4] = {
        new int[10000],
        new int[10000],
        new int[10000],
        new int[10000]
    };

    for (int i = 0; i < 8; ++i) {
        Bitplane[0][i + 24] = oDataK[32 * 17 + 14 - i * 2 + Start_Of_First_Tile];
        Bitplane[1][i + 24] = oDataK[32 * 17 + 15 - i * 2 + Start_Of_First_Tile];
        Bitplane[2][i + 24] = oDataK[32 * 17 + 30 - i * 2 + Start_Of_First_Tile];
        Bitplane[3][i + 24] = oDataK[32 * 17 + 31 - i * 2 + Start_Of_First_Tile];
    }

And in this loop, the j is pointless. Just use N.

            for (int j = 0, N = 7; j < 8; j++, N--) // not this
            for (int N = 7; N >= 0; N--)            // just do this
            for (int N = 8; N-- > 0; )              // Or this (which also works for an unsigned variable) 

Instead of this

    for (int p = 24; p < 32; p++)
    {
        // Bottom Right of 16x16
        if (p == 24) { c = 256 * (y_coord + 15) * 4 + x_coord + 0 + 0;; }
        if (p == 25) { c = 256 * (y_coord + 14) * 4 + x_coord + 0 + 0;; }
        if (p == 26) { c = 256 * (y_coord + 13) * 4 + x_coord + 0 + 0;; }
        if (p == 27) { c = 256 * (y_coord + 12) * 4 + x_coord + 0 + 0;; }
        if (p == 28) { c = 256 * (y_coord + 11) * 4 + x_coord + 0 + 0;; }
        if (p == 29) { c = 256 * (y_coord + 10) * 4 + x_coord + 0 + 0;; }
        if (p == 30) { c = 256 * (y_coord +  9) * 4 + x_coord + 0 + 0;; }
        if (p == 31) { c = 256 * (y_coord +  8) * 4 + x_coord + 0 + 0;; }
        ...

maybe do this:

    for (int p = 24; p < 32; p++)
    {
        // Bottom Right of 16x16
        c = 256 * (39 - p + y_coord) * 4 + x_coord;
        ...

Also, what you are calling "white" in your color table has the rgb values for black, but since it's alpha is 0 it's actually just transparent. Do you mean it to be transparent?

EDIT:
I just noticed that in the code you've posted you never set the variable X.

Also, instead of using bitset, you can just do this:

    unsigned Index = 0;
    for (int i = 4; i-- > 0; ) {
        Index <<= 1;
        Index |= (Bitplane[i] >> 5) & 1;
    }

[George PlusPlus]

#include <atlstr.h>

Are you actually using the Visual Studio Active Template Library component? The ATL's used to simplify using COM objects and creating ActiveX components. Windows type stuff that really isn't C++ stdlib related.

Trying to read/write image files by using ATL is a bit of overkill even if the library is used correctly.

[jonnin]

be careful reading off the desktop as well. I have had permissions troubles on that, though it depends on how your machine is configured. If it works, fine, but be sure you are fully checking that the file opened successfully in case of any surprises.

if you have additional troubles after the above fixes, consider writing the image file back out in a new file to be sure you read it in correctly, as a tmp.png or something that you can open in an image editor. This basically proves that you read it in properly and got the one you wanted...

[seeplus]

Are you actually using the Visual Studio Active Template Library component?

CString is used which is from atlstr.h. CString should be replaced with std::string.

int* Bitplane0__ROW = new int[10000];
int* Bitplane1__ROW = new int[10000];
int* Bitplane2__ROW = new int[10000];
int* Bitplane3__ROW = new int[10000];

This is just an array (4 elements) of an array (10000 elements). As the size is fixed at compile time, std::array can be used with smart pointers so that allocated memory is automatically released. Something like:

#include <array>
#include <memory>

constexpr size_t plane_size { 10000 };

....

std::array bitmaps { std::make_unique<int[]>(plane_size),
      std::make_unique<int[]>(plane_size),
      std::make_unique<int[]>(plane_size), std::make_unique<int[]>(plane_size)};

or could simply use vectors:

std::vector bitmaps(4, std::vector (plane_size, 0));

Why use CString and char* when std::string should be used?

.seekg() should use a specified direction constant rather than a number
https://en.cppreference.com/w/cpp/io/ios_base/seekdir

Also there is no checking that the file has been opened OK.

https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique
https://en.cppreference.com/w/cpp/container/array

[George PlusPlus]

@seeplus, I know where CString is from, I was kinda hoping the OP would explain why they might be using it instead of <string>. :)

Along with a number of design choices that could be made more bullet-proof. C++ containers vs. C-style arrays and new for instance.

[TheIdeasMan]

Also, don't include cpp files :+)

Try to use const and constexpr more.

using namespace std; will bite you in the ass one day, just put std:: before every std thing. Plenty written about this on internet.

Always try to declare and assign a value to a variable in 1 line of code. Wait until you have a sensible value, zero is not always a good value. This will avoid problems with variable X

[George PlusPlus]

TheIdeasMan wrote:

Also, don't include cpp files :+)

Ack! I missed that! Oooopsie!

I suspect so did everyone else until now.

Wanna bet removing any #included .cpp files will resolve a whole bunch of problems? :Þ

Always try to declare and assign a value to a variable in 1 line of code. Wait until you have a sensible value, zero is not always a good value. This will avoid problems with variable X

Another suggestion, declare a variable as close to first use as possible, instead of the C way of declaring all variables at the head of the function.

Not this:

#include <iostream>

int main( )
{
   int answer { 42 };

   // lots and lots of boiler-plate code that doesn't use the variable

   // I do mean LOTS of code

   std::cout << "The answer to life, the universe, and everything is "
             << answer << ".\n";
}

Do this:

#include <iostream>

int main( )
{
   // lots and lots of boiler-plate code that doesn't need the variable

   // I do mean LOTS of code

   int answer { 42 };

   std::cout << "The answer to life, the universe, and everything is "
             << answer << ".\n";
}

About using namespace std;...It is not wrong, it is not bad to use, but as mentioned it will cause problems later when you start writing your own libraries and choose commonly used designations you might not know are part of the C++ stdlib.

https://stackoverflow.com/questions/1452721/whats-the-problem-with-using-namespace-std#1452738

Yes, it means a couple of more key strokes are "saved" but after a while always typing std:: becomes second nature. It has become so ingrained into my programming headspace to NOT type std:: slows me down. Waaaaay slower.

On a side note....prefixing std:: makes it explicit what I'm using, something from the C++ stdlib. Enhancing readability never hurts.

In the long run saving a couple key strokes is not worth the effort IMO, my fingers prefix automatically.

[TheIdeasMan]

There is another way to help with std:: , depending on one's IDE / editor : keyboard shortcuts (is that the correct term?). One can use whatever menomics they wish, but say that sveci means std::vector<int>, type sveci and it is replaced with std::vector<int>. That is a saving of 11 keystrokes, more if it is std::unordered_multimap, and even more still if one uses their own class type often. I guess one downside is having to remember all the shortcuts, but one could get used to that too, provided the naming is consistent and logical.

Muscle memory can be quite hard to undo: For a good 30 years I was always in the habit of first typing both opening and closing braces (or any other that come in pairs), then going back to put something inside. Now editors will do that auto-magically, although some of them are smart enough to recognise that behaviour and not do 2 closing braces.

[Cyclone]

Thanks everyone for taking the time to give me advice.

I know my code is horrendous but I'm still learning although slowly. :(

My code is messy and had left over variables from testing and trial and error. My code needs revising as you guys mentioned putting things into arrays. All makes sense.

btw the program code above does not give any errors but the full program(as mentioned in my previous forum topic.) still does.
and I fixed the issue with the tiles not writing correctly. There was a slight difference in the bytes of the game file.

Thanks again!

[TheIdeasMan]

Cyclone wrote:

My code is messy and had left over variables from testing and trial and error.

Try using a version control system such as git. There are GUI plugins which are easy to use. Put your whole project under git control, check out files, make small changes - even if it is just one function, compile and fix until it works nicely, commit changes with a comment on what you did. Then it is possible to revert to an earlier version. One can do branches.

When you say: " ... have left over variables from testing and trial and error. " Consider that it is easier to use the debugger to see what is happening with variables during program execution, rather than inventing new variables, then printing them to see what their value is. One doesn't have to spend time going back to get rid of those extra variables. One can notice straight away uninitialised variables like your variable X - provided that you added it to the watchlist, because they will show garbage values.

Btw, X is a terrible name for a variable. If one uses good names for variables, functions and classes, reading the code should be like reading a story of what is going on.

The variable X should be of type std::size_t because it is an array subscript, and a sensible initial value for it is zero in this case, because you are writing to the start of the array. std::size_t is usually the largest unsigned type the system has, but this is not guaranteed by the c++ standard. On my system it is unsigned long which is 64 bit on Linux, it is probably 64 bit on your windows system too, but it will be unsigned long long . One should use std::size_t with all the STL containers, for example the size() functions return a type of std::size_t, one can avoid a compiler warning when there is a comparison between int and std::size_t

Another thing about "messy code": there is a rule about how much code should be in a function, some folks go for as little as 25 Lines Of Code (LOC), the bombshell is that it includes the main function !! Part of the reasoning is that a function should only do one conceptual thing only. Higher level functions can call multiple lower level functions to do their work. The other reason is that it makes it easier to manage the project. Once a small function has been proven to work, one does not need to worry about it any further, apart from what arguments were sent to it, but the function should validate them anyway.

About design: People seem very reluctant to write down / draw their design on paper. Consider writing into a file a series of comments which describe what one wants to do. One can identify what needs to be in a class, what code could be in a function - loops say, what code will be in different files etc. Then go back and write the code which does what was in the comment.

When coding, write a small amount - even if it is just 1 function and the call to it, compile and test to make sure it works before you continue. This way better than charging in and writing 100's of LOC, then wonder why there are lot's of errors.

This guide is written by Bjarne Stroustrup and Herb Sutter, you may find it useful to look as your coding progresses :

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md

Finally I do hope you are using an IDE? IMO it is much easier for beginners, because of things like intelli-sense which show problems as one types. Perhaps one could learn more by using a plain editor and the shell / command line to compile, but one really does learn the hard way, which can be frustrating.

Anyway Good Luck !!! :+)