Paint tool issue

The Partridge Family were neither partridges nor a family. Discuss.
Post Reply
User avatar
Radical
Posts: 38
Joined: January 15th, 2017, 9:16 pm
Location: Ontario

Paint tool issue

Post by Radical » March 25th, 2018, 3:58 am

I had the idea to create my own Microsoft Paint clone today. I whipped up some quick code just to get the concept down and make sure my idea worked as intended. Everything seems great! ...unless I change the size of the array.

The array is the number of painted pixels I can have on the screen. I have it set to 10000 right now, and it is all gucci. But if I lower it to say, 100, the second I click to place a single pixel, everything crashes. It is very simple code, and it should have no reason to behave like this. I can't find the source of the error anywhere.

I will include the main code if you want to look at it, but you may still need to download the actual solution file:

Code: Select all

if (wnd.mouse.LeftIsPressed())
	{
		p[i].x = wnd.mouse.GetPosX();
		p[i].y = wnd.mouse.GetPosY();
		i++;
	}

	for (int a = 0; a <= i; a++)
	{
		gfx.PutPixel(p[a].x, p[a].y, p[a].r, p[a].g, p[a].b);
	}
I could just say "screw it" and accept that it works at the higher numbers which I will be using anyway, but I feel this has a high chance of biting me in the butt later on down the line.
Attachments
Paint.zip
(86.46 KiB) Downloaded 114 times

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Paint tool issue

Post by albinopapa » March 25th, 2018, 7:36 am

Code: Select all

if (wnd.mouse.LeftIsPressed())
   {
      p[i].x = wnd.mouse.GetPosX();
      p[i].y = wnd.mouse.GetPosY();
      i++;
   }

   for (int a = 0; a <= i; a++)
   {
      gfx.PutPixel(p[a].x, p[a].y, p[a].r, p[a].g, p[a].b);
   }
There are a few problems with your approach.

Your i variable is never reset to 0, which means once it reaches more than your set array size -1 (999 or 9999) then you will crash.
You are incrementing i even when the mouse is over the same pixel, so your buffer will fill up fast with redundant data.
You are adding pixels to the buffer at 60+ frames per second, so the buffer will fill up fast no matter what.
The framework is designed to refresh the screen each frame, so unless you keep the data in the buffer, your image will never show.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Paint tool issue

Post by albinopapa » March 25th, 2018, 7:38 am

Code: Select all

class Game
{
public:
	Game( class MainWindow& wnd );
	Game( const Game& ) = delete;
	Game& operator=( const Game& ) = delete;
	void Go();
private:
	void ComposeFrame();
	void UpdateModel();
	/********************************/
	/*  User Functions              */
	/********************************/
private:
	MainWindow& wnd;
	Graphics gfx;
	/********************************/
	/*  User Variables              */
	static constexpr size_t numPixels = 10'000;
	Pixel p[ numPixels ];
	int i = 0;
	int amount = 0;
	/********************************/
};

Code: Select all

Game::Game( MainWindow& wnd )
	:
	wnd( wnd ),
	gfx( wnd )
{
	for( auto& pixel : p )
	{
		pixel = Pixel();
	}
	gfx.BeginFrame();
}

void Game::Go()
{
	//gfx.BeginFrame();	
	UpdateModel();
	ComposeFrame();
	gfx.EndFrame();
}

void Game::UpdateModel()
{
	// Reclaim the pixels in the buffer by resetting i
	i = 0;
	if( wnd.mouse.LeftIsPressed() && i < numPixels )
	{
		const auto mx = wnd.mouse.GetPosX();
		const auto my = wnd.mouse.GetPosY();

		// only search from &p[0] to &p[i]
		const auto it = std::find_if( p, &p[ i ], [ mx, my ]( const Pixel& pixel )
		{
			return mx == pixel.x && my == pixel.y;
		} );
		
		if( it == &p[ i ] )
		{
			p[ i ].x = mx;
			p[ i ].y = my;

			// Assign whatever selected color is
			p[ i ].r = p[ i ].g = p[ i ].b = 255;
			++i;
		}
	}
}

void Game::ComposeFrame()
{	
	for( size_t j = 0; j < ( size_t )i; ++j )
	{
		gfx.PutPixel( p[ j ].x, p[ j ].y, Color( p[ j ].r, p[ j ].g, p[ j ].b ) );
	}

}
This is just one possible solution, though a naive one.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

User avatar
Radical
Posts: 38
Joined: January 15th, 2017, 9:16 pm
Location: Ontario

Re: Paint tool issue

Post by Radical » March 26th, 2018, 11:40 pm

This is some good shit papa. I tried out your code and lowered the pixel array size to 1, but it still works with an infinite amount of pixels. I'll be honest though, I don't know wtf is going on. You seem to be dealing with actual memory storage/allocation (or whatever it might be called), and I have no experience with that yet. I don't know what half the things in the code even do. For example what auto is, what std::find_if is, what : is, what size_t is, and other things such as why you needed to move the call to BeginFrame(). I tried to do a bit of research but didn't receive an easy answer and felt demotivated because it was all over my head.

Is this something chili covers in later tutorials? Do you feel I should try to power through now and learn it, or let it happen in due time? I always am hesitant to stray too much from the tutorials, as it feels like most things have prerequisites. I mean, a little while back you did help me learn about references before chili covered them in the tutorials (and that helped out with my coding a shit-ton), but they are a pretty simple concept.

Slidy
Posts: 80
Joined: September 9th, 2017, 1:19 pm

Re: Paint tool issue

Post by Slidy » March 27th, 2018, 12:21 am

Most of this is covered by Chili later in the intermediate tutorials, but the same thing can be accomplished even without these (slightly) more advanced tools. I'll try to cover all of them anyway.

auto
auto is a C++ keyword that basically means "try to guess what this type is"
for example if I did:

Code: Select all

auto f = 0.5f;
the compiler can tell that this is meant to be a float, since you're assigning a float value to it. This is exactly the same as doing:

Code: Select all

float f = 0.5f;
except with auto you don't explicitly type out what the type is.
This wouldn't work unless the compiler can tell what your type is meant to be. So doing:

Code: Select all

auto f;
wouldn't work since the compiler can't tell what the type is meant to be, it can be anything.

This is especially helpful when dealing with long or difficult names such as std::vector<SomeVeryLongDumbName>::iterator, you wouldn't want to type all that out so you just use auto

std::find_if
This is part of the <algorithm> library. It's a little difficult to explain how it works exactly without knowing about iterators and functors/lambdas, but the basic gist of it is that it loops through all the entries in a container (like an array or a vector) and finds an element based on a certain condition.
The same thing can be accomplished with a basic for loop, heres an example that finds the first even number in an array:

Code: Select all

int arr[50];
for( int i = 0; i < 50; i++ )
{
    if( arr[i] % 2 == 0 )
    {
        // found it!
        // do whatever
    }
}
:
I'm pretty sure this is covered early in the vids, but its a range based for loop. It's a shorthand way of looping through all entries of a container.

Code: Select all

for( int some_int : some_vector )
{
    // whatever  
}

// equivelant to

for( size_t i = 0; i < some_vector.size(); i++)
{
    int some_int = some_vector[i];
    // whatever
}
size_t
This is a very simple one, its basically just another way of saying for "unsigned int" (an int that can only be positive), but typing that out is long and tedious, so size_t is an easier way of doing it.
It's used a lot by the standard library things like counts/sizes which obviously can't be negative. You could also use a regular int, but doing this without a cast would give you a warning.

Code: Select all

for( int i = 0; i < some_vector.size(); i++ ) // would give warning, assigning unsigned to signed
for( int i = 0; i < int( some_vector.size() ); i++ ) // would not give warning, but ugly
for( size_t i = 0; i < some_vector.size(); i++ ) // would not give warning, easy and nice

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Paint tool issue

Post by albinopapa » March 27th, 2018, 12:38 am

First let me go through some stuff that's pretty easy to figure out.

Code: Select all

   static constexpr size_t numPixels = 10'000;
   Pixel p[ numPixels ];
   int i = 0;
   int amount = 0;
In the tutorials, chili does explain what constexpr is, don't remember at which point, but it is just a new-ish keyword for C++ since 2011 that allows you to declare variables and assign to them constant values, like the 10'000. The compiler will replace the variable name with the constant during compilation instead of looking in memory for the value.
Because this constexpr variable is declared within the Game class, it needs to be static. When declared like this, it's not stored with the instantiated object, as stated, if it's a simple int or float the variable gets replaced during compilation. In this context, static means it'll have the same value for every instance of Game...which will only ever be one in the chili framework.
Since it is a compile time constant, you can use it when declaring static C-style arrays, Poo p[ numPixels ];

I believe in C++14, you can use the apostrophe to separate digits and it gets ignored by the compiler. It's just a nice way of letting the programmer know what the number is without having to count 10000 vs 10'000.

The type size_t or std::size_t is just an alias for unsigned int when compiling for x86 ( 32bit ) and unsigned long long when compiling for x64 ( 64 bit ). A lot of standard library functions that return a size ( ie, std::vector::size() ) returns a size_t because the size of something will never be negative and a type with size in the name kind of explains what data it holds. I probably use it more often than I should. Unsigned int or uint32_t would work, but as I said, it's an unsigned long long or uint64_t when in 64 bit mode. So instead of switching back and forth between uint32_t and uint64_t, I just use size_t for array indexing or size type parameters.

Code: Select all

   for( auto& pixel : p )
   {
      pixel = Pixel();
   }
   gfx.BeginFrame();
In C++11, they introduced the auto keyword. This allows you to 1) not need to know what type p is and 2) shorten variable declarations by using auto instead of the fully qualified name. An example would be pre-C++11 people used iterators in their for loops instead of indexes.

Code: Select all

for(std::vector<float>::iterator it = vec.begin(), end = vec.end(); it != end; ++it)
{
     // Do stuff with the dereferenced iterator by prepending it with an asterisk
     *it += 3.42f;  // The operator* is overloaded for iterators to return a reference to the element it points to.  It's like a safe pointer.
}
// Same loop using auto to deduce what the type of it should be
for( auto it = vec.begin(), end = vec.end(); it != end; ++it)
{
     *it += 3.42f;
}
// Same loop, but using indices
for(size_t i = 0; i < vec.size(); ++i)
{
     vec[i] += 3.42f;
}
//C++11 range based for loop
for( auto& elem : vec )
{
     elem += 3.42f
}
As you can see, the first auto version is preferable to the full blown std::vector<float>::iterator version.

In your code, you use a C-style array or static array. It's size is known to the compiler, so the use of the range based for loop is permitted. This will not work if you allocate memory from the heap. Chili does cover memory allocations in the first couple episodes of Intermediate.

I put the gfx.BeginFrame call in the Game::Game function to just clear the screen to black once. Calling it each frame would be a waste because you aren't wanting to clear the screen at 60 fps while trying to draw. Ideally, you might put this in a function that gets called when you start a new image.

Code: Select all

   // Reclaim the pixels in the buffer by resetting i
   i = 0;
   if( wnd.mouse.LeftIsPressed() && i < numPixels )
This resets i back to 0 each frame, this way it doesn't just keep increasing forever. You only have so much memory to deal with and you can only allocate a small C-style array on the stack...about 1 megabyte or less. Probably anything greater than or equal to a 512x512 image would cause a "stack overflow".

Code: Select all

     // only search from &p[0] to &p[i]
      const auto it = std::find_if( p, &p[ i ], [ mx, my ]( const Pixel& pixel )
      {
         return mx == pixel.x && my == pixel.y;
      } );
Here, &p[0] just returns the address of the first element of the array same as just putting p and &p gets the address at p. p at index i is the last inserted Pixel object.

The std::find_if is a function that loops through from p to p + i looking for matching x and y values to the mouse x and mouse y values. The std::find_if function takes as it's last parameter a predicate function. In this case, I use a lambda function ( also introduced in C++11 ).
A lambda is a function object that can be declared and defined inside of another function. Chili does briefly cover lambdas in the intermediate series...actually all of the things I've used here are introduced or more thoroughly covered in the intermediate series.
There are three parts to a lambda: the capture list, the parameter list and the function body.
The capture list is a list of variables between the square brackets [ ].
The parameter list and function body are the same as any other function.

The capture list can capture specific variables like the [mx,my] I did here. Or capture everything that it uses.
To capture by value, just use the name of the variable like the [mx,my].
To capture everything by value, use the = sign. [ = ]
To capture everything by reference, use the reference operator &. [ & ]
You can and sometimes must capture the 'this' pointer. [ this ]

If you use the = to not have to list a bunch of variables, only those that you use will be copied.
If you use the &, only those that get used will be referenced.
If you want to access a member variable of the current class, you must pass the this pointer.

Code: Select all

      if( it == &p[ i ] )
      {
         p[ i ].x = mx;
         p[ i ].y = my;

         // Assign whatever selected color is
         p[ i ].r = p[ i ].g = p[ i ].b = 255;
         ++i;
      }
   }
The std::find_if function like most standard library algorithm functions, return an iterator. In this case, a pointer. For this project, you want to check if there was a match and if there is, than that pixel is already in the array, so no need to store and increment i. Since index i is the last element inserted, we want it to be the same as i, meaning the pixel is not in the list. If it's not in the list, let's go ahead and assign a new pixel and index i and increment i to get ready for the next insertion.

Remember, when you click the left mouse button, several frames go by during that click. You don't want to store the same pixels unless the color has changed or you are using a different tool like the eraser for instance.

I didn't do any advanced heap allocations or other memory management, I still used the same C-style or static array you did. I just made sure that the index i didn't go over the pre-allocated or stack allocated amount of space.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Paint tool issue

Post by albinopapa » March 27th, 2018, 12:51 am

@Slidy, lol, I like your explanation better. Short and to the point. Very clear.

One thing to note here is in Slidy's explanation of auto he uses std::vector<SomeVeryLongDumbName>::iterator. The angle brackets means template parameter and those parameters need to be a type not a variable. In this case, SomeVeryLongDumbName would be a class or struct for instance.

The only exception would be in some cases, you can use compile time integral constants like declaring std::array. In std::array you have to provide a type and a size as template parameters.
std::array<float, 420> farr;
This declares an array of 420 floats.
std::vector, however, always wants a type like float/int or a user type like SomeVeryLongDumbName.


Chili kind of covers templates later in the intermediate series as well. A lot of the intermediate series is topical and it's your job to experiment and learn the in depth, but hey you have us here to help at any time.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Paint tool issue

Post by albinopapa » March 27th, 2018, 1:04 am

Oh, aside from the coding, one thing you may or may not notice is the spaces left between pixels when drawing like this. My current mouse and previous one both did this. Cameron and I tested this out a couple years ago and it seems to depend on the dpi or resolution of your mouse. How often the hardware is polled or something. His mouse had way more pixels drawn than mine, so the pixels didn't need to be interpolated between. My mice have all been pretty cheap so it isn't polled as often.

You may have to interpolate between pixels to avoid leaving gaps. I just drew lines between the pixels and as long as I wasn't trying to draw too fast, it was hard to tell the difference.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

User avatar
Radical
Posts: 38
Joined: January 15th, 2017, 9:16 pm
Location: Ontario

Re: Paint tool issue

Post by Radical » March 27th, 2018, 5:06 am

@Slidy omg is that what unsigned ints are? Here I was thinking they were some super complex thing. I kept hearing the term thrown around and I assumed an unsigned int was an incognito int, with no signature to it's name; untraceable or something along those lines.
I have now tried out size_t for myself and I gotta say... that's super useful.
Range-based for loop was also well explained.

Thanks to you both; some very good info. I was consideri-WAIT A MINUTE. I didn't realise the only purpose of BeginFrame() was to refresh the screen. Leaving BeginFrame() to only be called once, I just deleted all the code and replaced it entirely with

Code: Select all

if (wnd.mouse.LeftIsPressed())
	{
		gfx.PutPixel(wnd.mouse.GetPosX(), wnd.mouse.GetPosY(), r, g, b);
	}
Everything works exactly the same. Now I'm questioning why such complex code was needed to perform the same operation.


Nevertheless, I tried to understand all that you wrote. It took me a while. (Longer than normal. I just had knee surgery so I can't exercise or move at all. If I don't exercise I can't focus.) While I haven't fully grasped the concepts of each individual thing, I think that's okay. I still gained a fair bit of knowledge out of it. I don't think I'm done anyway. I will come back to this tomorrow, and if I have any questions, I'll ask.
That's one of the things I like about this forum. Every time I post a topic I always end up learning something. (Even if it's not what I was originally intending to learn when I asked for help haha)

@ablinoppa Yeah, don't worry, I know there's gaps in between the pixels. The code I posted here was just rough concept work, literally to see how I would be able to place a pixel on the screen, leave it there, then place another one. I will be going forward with the project and fleshing it out. Full nice brush strokes, an eraser, colour pallet, or whatever else my heart desires.

albinopapa
Posts: 4373
Joined: February 28th, 2013, 3:23 am
Location: Oklahoma, United States

Re: Paint tool issue

Post by albinopapa » March 27th, 2018, 9:55 am

Everything works exactly the same. Now I'm questioning why such complex code was needed to perform the same operation.

It wasn't needed.

A few things you might have to think about if you choose to do things this way would be:
how to undo an operation,
how to do multiple layers,
how to handle transparency,
how to handle drawing shapes like ellipses and rectangles, lines and curves,
how to handle gradient and bucket fill

I'm not saying your original approach was the right way to go either, however, it afforded some versatility as opposed to drawing straight to the system buffer.
  • For instance, if you wanted to undo a previous operation with your first approach, you could maintain those pixels draw and just pop them off or decrement index i.
  • If you added a layer number to Pixel, you could sort them so that layer 0 pixels are drawn first then layer 1 and so on.
  • You have the r,g,b channels already separated and if you added an alpha channel you would be able to do the calculations for blending colors from the multiple layers.
  • You'd be able to just iterate through the pixels to make outlines of shapes.
  • You could then use those lists to scan through and do a bucket fill line by line connecting the dots so to speak.
  • You can use the pixel list to create edges for a cut operation.
I think there are better ways to approach the problem. Go down this path and see how far it takes you. Try other methods and see which works out. Some might be better for memory efficiency, some might offer better performance in terms of responsiveness, some might be better for flexibility. There isn't just one path to take.
If you think paging some data from disk into RAM is slow, try paging it into a simian cerebrum over a pair of optical nerves. - gameprogrammingpatterns.com

Post Reply