LEARNING C++ with Java/C#/Python Experience // Code Review

hey what's up guys my name is ashana welcome back to another episode of churno fixes your code i'm in code review this is the code review series right this of course is a series in which you guys send me your code and i take a look at it i give my thoughts i give some suggestions to people learning programming and all that and hopefully it's just a good time for everyone involved last time we took a look at a raytracer i'll have a video linked up there if you are interested and today we're going to be diving into a little bit of a blast from the past we're going to be talking about a game that i actually remember from windows vista except someone's made made it for some reason i don't know if you remember inkball but it was a game that came bundled with windows vista i made a version in c plus plus with sdl2 i have a fair bit of work and open source experience but not very much in c plus plus more c-sharp javascript typescript python java experience this is actually something that made me want to take a look at this project because i know a lot of you are familiar with other languages you're not just learning programming for the first time i think that it's probably a minority who have never done any programming before and they're starting off with like c plus plus or c i mean i'm aware that that does get taught in university like a lot i remember learning c in my introduction to programming class however these days if you're self-taught i feel that if you're into games you're very likely using c-sharp with unity or trying to make some kind of java c-sharp game kind of stand-alone or if you're into like web development and that stuff you're probably using like javascript python that kind of stuff in fact python is very much useful outside of web dev i know a lot of people who started learning how to make games using pie game and python so the point being that there's all these different languages out there and if you're more or less familiar with them but you're coming to c plus plus there's a lot that you might may have to learn in fact this is my personal experience as well i started learning programming with java and i used that for several years before working at ea using c plus plus there are a lot of differences between those languages and there's a lot to learn when you come into c plus plus so i'm excited to take a look at benjamin's code ultimately and see if maybe i can give some good tips to all of you guys who are also in the same boat project is completely playable includes five levels in a homemade binary format that is decentralized runtime that's cool it doesn't use a game engine and it was more project for learning than anything else okay cool i wanted feedback on my seatbelts and project architecture especially the way i handled the civilization and manage the lifetimes of sdl objects okay so those are two things that we will take a look at if i were to continue development i'd probably uh better highlight the separation between level and playable level again we'll find out what that is in the future as well as set up a less finicky build process yeah the build system is definitely something that is difficult to kind of wrap your head around if you're coming from different languages because c plus plus build system is well it doesn't really have a build system and everything else that it has is kind of all over the place so we'll take a look at that as well currently all dependencies are statically linked that's good and it's compiled uh with mingw on windows okay we might try and use visual studio because that's what i'm gonna do you may be aware that mingw doesn't support a city thread very well this introduces some compilation headaches yeah you should probably just use visual studio i apologize if you're on windows right i apologize for the difficulty in compilation okay and if you do decide to do a code review i would not be offended if you don't run the game i don't even think i have multi through any multi-threaded code anyways okay so no need to really have threads i guess there is no need from a performance perspective okay for multi-threaded code i see okay the link to the repository inkball let's take a look for those of you who don't remember this this is this is inkball this is the game that we are talking about that i remember and when does this are in fact this logo over here brings back memories so taking a look at the repository which i will have linked in the description below uh let's read the readme it's an inkball game that's still in development it doesn't need a game okay cool it looks very similar i'm working i have working collisions and you can draw an ink trail which interacts which balls interact with that's also cool to do level creation maybe building these are not full instructions all the pencils are linked gcc 17 you can see dependencies independence take a look at that and uh okay so cmake okay so it's built using cmx so we should have no problems hopefully running this inside visual studio needs sdl to sdl image and ttf mingw threads on windows as well test it with 32 bits of all libraries for sporting okay i wouldn't recommend really working with 32-bit these days i mean i would just just go with 64-bit for everything pretty much we're in 2021 but aside from that i'm excited to take a look but first i want to mention that this video is of course sponsored by skillshare for those of you who still don't know what skillshare is skillshare is an amazing online learning community where millions of creative and curious people from all around the world can come together to browse their incredibly dense and beautiful library of classes on pretty much any topic and learn new skills ultimately there are two things that i really love about skillshare first of all the the variety of classes they have i mean literally anything you're interested in photography videography like illustration even stuff like business management productivity marketing they literally have videos on absolutely everything but also it's not just about quantity the quality of these videos is also exceptionally high they're really kind of short and bite-sized they're easy to digest they're easy to get through even if you don't have much time left in your day and it's kind of formatted in a way that just keeps you learning it's really easy to take a look at like a 30 minute video and just put that on the back burner because you just you can't seem to find the time to sit down for that amount of time but skillshare's format is mostly extremely bite-sized meaning a lot of the videos are like 5-10 minutes long which of course makes it easy to just find some time during a break sit down and keep yourself constantly learning which honestly has so many benefits for your brain skillshare is offering the first 1000 people who click the link in the description below a one month free trial of skillshare premium where you'll be able to literally take a look at any of their classes and see if you like them or if there's something there for you so definitely check them out go ahead and learn a new skill and huge thank you as always to skillshare for sponsoring this video so as usual the first thing that i'm going to do is clone it i'm not going to bore you guys with the details if you have no idea how to obtain like repositories from github and all of that stuff take a look at the other code review videos because they go through the setup process really well and i'm probably gonna start skipping it from now on because i think you guys get the point quick little tip though if you have accidentally closed the explorer window like i did and you still have the command prompt open you can just type in start dot which will just open a new explorer window in the current directory which is very useful let's go ahead and c make this c make error at c make lists add a subdirectory a given source okay so i actually i'm going to focus a little bit on the build system stuff just because this person mentioned that they are coming from you know other languages into c plus plus and this of course is very relevant to c plus plus and will hopefully help a lot of you guys out so as subdirectory given source so what's what's going on here is cmake is trying to obviously add this directory as another kind of cmake project but it clearly just does not exist or at least there's well it's not an existing directory in this case so this is a little bit unfortunate because i mean we know from the dependencies that mingw threads specifically seems to be an actual dependency not like the rest of mingw i guess i am going a little bit off the book here because i'm trying to compile this with visual studio however yeah it you know it should obviously exist here and it does not so what can we do about this so over here it looks like we have this mingw sc threads kind of repository which looks like it is a sequence 11 threading class implementation which are still currently missing which is what benjamin was referring to it's also a header only library so this is something that we would ideally want to use in our repositories and i imagine that's probably what the original author of this kind of inkball benjamin this is ultimately probably what he was using so let's talk a little bit about how we can actually set up this dependency in cpl plus a little bit better instead of just obviously providing people with a project where they download it they run it and suddenly this does not exist this in fact is a perfect use for git sub modules so what a sub module is inside git is basically just another repository an external git repository that you can include as part of your project so you're basically just creating a link to another git repository within your repository the code isn't actually stored as part of your repository it's external and of course you have some extra levels of control there you can pin a specific revision like you don't have to be always on the master branch on latest code whatever you can have a different branch of the repository you can have a specific commit that's all perfectly normal so what i recommend you do for this is just add this repository as a sub module now of course you might be using actual mingw if you've installed that on your system maybe you don't need to do anything like this however this is a good example to kind of show how to do this so if we go back a directory i'm just going to type in git submodule add now we know here that because add subdirectories is used it's actually looking for a cmake kind of project right this has a cmakelist file so that's kind of important because that's that means it's going to work so back over here i'm going to do git submodule add i've pasted in this url and then i'm going to give it a folder or like a path to basically check out this repository into so we know that it's going to be somewhere here i mean we're already in inkball so just basically include mean gw is where we want to check out this repository let's hit enter aha the following parts are ignored by one of your git ignore files so it looks like this person has deliberately not committed this let's take a look at the get ignore and sure enough we have the include directory being ignored okay so i think this is the case of this person has checked out the repository but doesn't want it to be committed as part of their actual like inkball repository but again this is a perfect use for sub modules so what we're going to do is just get rid of include from here and like i mean well we'll keep lib there i guess and then we'll try and do this again it's going to clone all of this we should now have that at this path in fact if i just go ahead and open that real quick you can see we have this entire mingw threads repository so let's try and run cmake again we'll probably have to like delete the all the stuff here so let's go ahead and use that wonderful start dot thing and let's go ahead and uh just run cmake again okay so no more errors and we in fact have build files that are written so yeah that is how sub modules work now the actual sub module uh information goes into this git get modules file so if we go ahead and open that real quick and i'll just drag this in you can see that we actually have this include mingw and it's set over here and if you had like a particular branch all the information will kind of go over here and to obtain that if you're cloning the repository once you've actually pushed the fact that you even have a submodule you'll need to do that kind of recursive thing that's what i usually do when cloning any repository just in case they have sub modules you can just clone recursively to make sure that you also retrieve all of the sub modules okay cool so now that we have those build files let's actually go ahead and try and compile this with visual studio okay so it's actually automatically created x64 as the target architecture let's just go ahead and hit build and we have a whole bunch of errors because i can't find sdl so i'm assuming there's no sdl in this repository either all right i'm not going to bore you with this you should also add sdl as like a sub module or maybe even commit the built binaries if you want for each platform that you support i'm gonna go ahead and just download the binaries for sdl and the header files for sdl set all this stuff up and we'll be back okay so i've gone ahead and added sdl all of that seems to be okay now however we have a bunch of other errors let's take a quick look at them as a learning example so first of all use of designated initializes requires at least six plus latest i'm not sure if this is something that is fine in uh in like gcc g plus plus however to use like dot x and all of that stuff um inside visual studios compiler it seems that we need to be using the latest uh c plus plus version so like c plus plus 20 basically so if we go into like c plus plus and um i never remember where this even is okay it seems to be in general see what's language standard you can see it's set to c plus 17 which he said that it supports but clearly it does not so we have to use c plus latest and this should these errors should go away quick little note as well compilers differ greatly i mean maybe not greatly in the grand scheme of things but there are definitely differences between compilers so make sure that if you are kind of working on a serious simple plus project especially if you support more than one kind of tool chain and you want it to be kind of uh you know multi-platform i mean even if you don't just as a sequel plus developer it's obviously better to not specifically use um you know things that are specific to a particular compiler and in fact i think i saw one other thing here so let's go ahead and try and compile this one more time i mean one of the things that separates like a junior sable plus developer from a senior sequel plus developer is the fact that the senior developers are a little bit usually more aware of the fact that you know there are other compilers out there and the differences between them because that stuff is definitely something that you don't typically come across if you're just working kind of in the beginning and you're not really that experienced okay illegal reference to non-static member here um oh okay so type of so type of um yeah i think there were errors to do yes the type off is not a valid template argument type of which i mean i think it's type of in like c sharp so maybe that's where that's kind of leaking in type of is a gcc like gps plus specific keyword right so it's not actually portable the portable versions as far as i'm aware is decal type right so if you use decal type i better not be embarrassing myself so decal type and i just looked this this up to be safe decal type is uh standard across c plus plus since c plus 11 and again what it will do is literally look up the kind of static compile time type of whatever num blocks is right so if num blocks is a un16t that's effectively what we're doing here we're just figuring out the numeric limits of un16t however of course uh well that's 64. because i'm used to writing that but you're in 16t right so instead of writing this we can kind of use a variable for that instead or at least look up the type of the variables that if we decide that this changes we don't have to go back and adjust all these numeric limits so in general it's a good idea but just make sure that you're of course using the more kind of portable keyword and the keyword that is c plus standard not just specific to gcc okay and the final error looks like cannot open mingw i don't think we need that because mingw threads is a header only library that we seem to be using so i'm just going to go ahead and go into the linker and probably just get rid of this and hopefully that will just work okay and we are good let's go ahead and try and run this okay we need all of the dlls so clearly they're not statically linked to these libraries okay so i've just pasted in literally all of the dlls that come with all the sdl libraries that we've used let's go ahead and hit f5 and we finally have this game oh wow that seems to be lagging a bit okay when i move my mouse it doesn't do anything oh what's this oh my goodness so game over really is game over because look at that memory leak escape for menu still happening select level one okay stabilized and then what happens when this goes in here memory leak time okay so that's one thing i think we can definitely fix uh seems to be some kind of just massive constant allocation of memory every frame in the menu screens which is a bit odd but other than that let's ignore that and see how long i can run this for without crashing but yeah the other odd thing that i've noticed is that every time i move the mouse the whole game freezes so that's a bit weird oh cool like okay so i can draw like that let's try and actually get this in it makes it kind of annoying to play i mean i don't know if that's a feature because like it doesn't seem to be like maxing out a core or something right wait what am i supposed to do with this why is it not let's try level four oh now this is like practically unplayable like this surely it's not meant to be like this i don't remember what the vista version was like um but other than that yeah like pretty cool game i guess um i'm interested in how the drawing is made i mean i'm assuming it's just a bunch of lines but anyway that gives us a few things to take a look at and also in my notes i have the um benjamin wanted feedback on several husband project architecture so we kind of talked a little bit about the seatbelts build system and then also the way i handle decentralization and manage the lifetimes of sdl objects i feel like managing lifetimes does need to chat considering that memory leak but also destabilization is something that we'll look at okay but the stuff that interests me right now is the memory leak because that's kind of an issue obviously and also the weird kind of freezing when we like move the mouse and again i don't know if that's a feature i mean it's possible it is but to me it seems like some kind of weird bug because i think it makes it really annoying to play uh so we'll definitely take a look at that okay sterilized level scene i don't know why we have this open oh because we fixed all the decal types okay well let's take a look at the general kind of structure and we'll work out ourselves into fixing that memory leak so okay we have the main it sets up a game on the stack and then hits game.play pretty pretty uh you know normal architecture this seems to be a very common um architecture which is fine i have nothing against this you know it's also worth kind of pointing out whether or not you actually need this and if you can maybe just have like a you know game play or like start game function and you don't need to like make an object out of it but again the reason why it's i think it's okay is because you know a lot of people coming from c-sharp java are obviously very much into object-oriented programming and i don't know i don't think there's a huge deal with that in this sense uh you know overall i think object-oriented programming maybe we should make a separate video on this but i think it's okay i just think that you can't rely on it too much and you should just be aware of the fact that not everything has to be op but you know i don't think op is specifically bad especially for a lot of cases like this okay anyway so we create a window i'm not gonna you know dwell too much on this kind of sdl code uh load level from path okay so i guess we can take a look at the decentralization so decentralized level information um first thing i'm going to do is actually take a look at what the uh at what the levels look like so they're they're in binary right so let's go ahead and just see if we can open this my favorite binary editor by the way is hxd i think i've mentioned this before um but let's go ahead and drag this in just so we can take a look at the format okay how big is this file uh looks like it's 1.18 kilobytes all right and then we have yeah ink level it looks like the header and then a bunch of random stuff here which i imagine is probably the positions of things so let's take a look at how we are actually processing that load level from paths decentralized level information we have a bunch of vectors blocks pockets balls i'm assuming we just read like a size argument so this takes in a buffer immediately so does that mean that we so level digitalization read takes a path from this why does it take this in that's interesting takes the game in because it needs the game well apparently it needs to pass the game into deserialize that's probably the target of the data maybe but anyway we have a max size which seems to be like 10 [Music] what's this 10 kilobytes this max size isn't actually used anywhere so i guess that's relevant okay we use a c api which is fine you know i would probably do read binary though rb um i'm not sure i don't remember the differences between all of these to be honest i guess r is also fine because it's not like it's text specifically maybe it's binary by default um we get the file size cool we make a new local buffer of that size and then we put into here so again you know reinterpret cast changing char pointer to unsigned char pointer that's not really necessary for anything i mean to be honest there's no reason why you couldn't have just set it up like this by default but the other thing is what i would recommend is instead of using unsigned char um just use un8t right so u int 8 underscore t it's the exact same type as an unsigned char in fact if we take a look at this it's going to actually be an unsigned char right but it's a little bit more portable right across platforms across compilers and also obviously it has the the actual number eight inside the type so you know how big it is and i think that's important right so i would change it to this and but again you know it's when you're dealing with pointers and memory the type that it is is kind of not really that important uh it's just it's you know it's important for like pointer arithmetic and stuff like that if you end up doing um you know advancing the pointer forward or doing any kind of calculation with the actual point of value uh however in this case you know it doesn't really matter and i would do this you don't need that reinterpreted cast okay so a few things that future churno notice me while editing this video is there's actually a number of problems here in this function first of all we're opening this file here but we're never closing it so f open gets called we have this fp which is local variable we read all the data but we never close it so as soon as we've read the data into the local buffer which happens here we should we can and should close the file by just calling f close that's very important otherwise the file remains open until basically our program terminates because this pointer then gets lost the other problem is this local buffer right this local buffer gets allocated over here using new and then passed into this deserialize function this deserialize function then reads from this buffer pointer but it doesn't delete it ever so this is another memory leak right we need to make sure that we either delete it over here however i probably wouldn't do that just to keep it simple instead what we could do if we go back here is once we have deserialized this we could in fact just delete this right you could just do delete local buffer and make sure that you're actually cleaning up your memory otherwise this is just a memory leak where every time you deserialize the file not only does the file remain open but you also just never free this memory until your program terminates i see you love throwing exceptions which not not something that i'm personally into but it's okay you do you uh okay deserialized let's take a look at this so again you know and then we reinterpret this into this you know there's a lot of stuff going on here like why take in a raw buffer you might as well take something in that is in this particular format again you know pointers pointers are pointers pointers are integers that contain memory addresses the data at that pointer is completely irrelevant to the pointer itself they're two different blocks of memory uh literally right again again with the exception it's okay so we try and figure out whether bytes is less than the size of level have a level header in which case obviously it's not even there i feel like these code review series like videos they're just getting deeper and deeper i mean i could literally sit here for hours taking a look at this i don't know if anyone will be interested in that so i might just kind of speed up a little bit but i don't know i'm always kind of after feedback as far as what this series is so definitely let me know in the comments uh if you think this is fine or if i should speed up okay bytes needed yes we figure out how much how many bytes we need to read i guess and then we yeah so we have the the gist of this is we have a buffer of memory which we've kind of cast into this header right so that we can actually interpret it like that might be worth making a little video about binary civilization in general but we basically go through this buffer now we have the head of the actual data so we've advanced the header amount and now we're looking at the like the actual information of what comes next in the file structure so in this case we seem to have block information right and block information just contains an x and a y right using alignment here which is nice as well but x and y and they're floats right again that means that we basically have a number of blocks that is defined in the header and then all we're doing here is we're iterating through and apparently making doubles out of them but this takes in doubles as well so i thought these are floats interesting oh well yeah i guess you're just casting them implicitly to doubles sure storing those flows again i don't know why their doubles here this sort of slows anyway maybe you're using doubles for some kind of extra precision and collision detection or something i'm not sure but i'd probably just use floats for everything that's just that's just how i roll okay so then we go through the pockets i don't know what pockets really are but same kind of situation and then based on the pocket color which is a an enum value one two three or four we basically set some kind of texture name and i guess that's a texture that we load yeah this is all pretty standard um i don't really have any problems with this i like the fact that you use structs a lot i think that when dealing with serialization in binary it's important to use structs like this right so instead of having to deal with all of these loosely you can just take that buffer you know that it's supposed to be like a header um and then you can basically just uh you know cast the buffer at a particular offset if needed in this case it's from the beginning of the buffer to be like a level header and then that's it you've basically already got all your struct with all of the accessible information so the fact that you're actually using structs here um you know extensively for everything is great i think that's a perfect way of doing it the only other thing that i would do because like when it comes down to it binary civilization is obviously very error prone yeah i mean like there's other stuff we could talk about like big endian little endian i don't think that's a huge concern you know if you're mostly working in like an x 86 64 kind of uh platform you'll be a little indian and you don't really have to worry about anything however the one thing i would probably do here is just make extensive use of assets right so just literally every step of the way assert everything make sure everything is correct you're throwing exceptions kind of instead which is okay but i just prefer asserts personally and it means that you can kind of take away all of this stuff and clean it up a little bit when you're actually building like in release and stuff again not that exceptions that don't get thrown are particularly expensive but it's just a little bit cleaner in my opinion and it helps you kind of assert a lot of conditions which you're doing anyway but otherwise it looks pretty good i mean you're feeling vectors with values returning them which may not be the best idea but i think that overall uh this is totally fine and uh yeah looks pretty good to me okay so let's get real is it just me or do you really want to find out what this memory leak is because that's the thing that's been bugging me so let's talk about how we can track down memory leaks so there's a few things you can do um i mean the simplest way probably is possibly i don't really do it this way that often but you can actually take a look at um you know memory usage so you can find memory lakes with this um using the performance kind of profiling tools inside visual studio let's have a go at doing that because i don't think i've done this very often oh yeah we'll get rid of this because i had two of them let's go back and yeah this is fantastic let's go back to our performance profile our memory usage and hit start there's a few other ways obviously and i could um kind of show them but i'm just interested to see so we need to do like a game over right i remember this actually i have definitely used this before um quite a bit so we can take a snapshot like i think before or after or something um and yeah there's the memory so let's take another memory snapshot and what's going to do is just show like where the memory is being allocated basically so let's kind of um stop collection here it's nice to do it before and after because you can kind of see i guess where like the memory has has gone to but i think that it should be probably good enough to just take one snapshot when the memory leaks happening or at least and at least it should show you know what has allocated a large amount of memory because clearly whatever has allocated it it's used more than the entire rest of the program like by an order of magnitude probably so it should be pretty easy so you can see that during this we had 9000 allocations here we have uh 58 000 allocations clearly a lot more let's take a look at that so we're basically just i don't know if we can look at a like a hot path here but let's go through this so you can see that most of the allocations by far are coming from the play function and this is basically your call stack right so it's coming from drawing which we kind of expect get string size interesting and then sdl ttf get string size ah so what we are probably doing if i had to guess is can i go to this function easily is uh we're probably loading the font file repeatedly or something i don't know what else would be causing a memory leak um let's go okay so what is this get string size let's just go to get string size and let's take a look at this okay so here we are opening the font inside this function and i guess never releasing it from memory oh it's happening here as well okay but here we close it okay right so here we close it here we don't so i'm assuming if we did this it would in fact fix the memory leak um obviously we don't want to be i'm just testing this to see if it fixes the memory leak or not how do i get a game over or something man this mouse thing is so frustrating is this this i please someone tell me if this is intentional i might send him an email i'll be crashed we crashed oh sorry we use it here wow journal review more like channel breaking people's code it was working before me yeah plea i need to probably send an email and figure out i just can't deal with this if this is intentional this is nuts okay the orange ball is heading in okay yeah that looks stable so if we take a look at this um obviously we're not allocating memory now let's have a quick chat about this though do not open font font files repeatedly here right now i can see why you might be doing that right i mean if we take a look at this we're not just opening a static font file right over and over again and then closing it right we are doing it at a particular size now the thing with these fonts is that again unless we're doing something moderately fancy like sign distance fields for our font rendering we actually need to rasterize our font at a particular size right so it's possible that multiple sizes are being passed into here however this again is something that you definitely should i mean you know if you have more time maybe for this it might be and if you're actually investing into this game and you're going to be developing it for a while or setting up an engine which i know this isn't but ultimately you probably want like a resource or an asset manager that would be able to take care of this but what you could do here at the very least is just cache this actual font right because again opening and closing it like apart from you know well in this case causing a memory leak it's just going to be very bad for performance right there's no need for us to actually do that so what we could do is cache it instead now again the problem is that we have a size here right so it's not as straightforward as just like you know let's just not you know if we were opening it at a constant size then that's easy we could do something extremely simple such as just make it like a static up here right and then if you make it a static up here i'm just failing today if you make it a static up here you can set it to null pointer and then just do basically like a lazy load which is us being like basically the first time it's used by whatever it's used by we don't care we don't need to explicitly initialize it we can just basically wait for it to be used and then the first time it's used we set it to something like this right and then of course we don't need to close it and if we wanted to we could close it as like part of our shutdown i don't know if it's even necessary to do this to be honest if you're just terminating the application your operating system will obviously reclaim all the memory but to be nice i guess you would want an initialization and a shutdown or something like that but anyway point being that uh you could just do that but that doesn't work for size because in this case this has nothing to do with size meaning that obviously it's possible that you might want um that you might want something a little bit different in the sense that like you might want like some kind of uh unordered um it doesn't matter unordered ordered map i'll just use a map to be a little bit cleaner but you could have your your size and then your actual pointer right and then all this is your little like font map and then that way you can check to see if if the key which is size exists within that map if it doesn't exist you load the font and you cache it in this map right um and then you just use it but if it does exist then you just retrieve the pointer from the map and obviously everything is fine right so you could do something like that and then you could loop over this map and close all of them if you needed to but that's that's kind of how you could make it work with all of this and again if you had an asset manager that would kind of handle all that behind the scenes all you have to do is ask the asset manager for this font for a given size and then that's kind of it's up to the implementation details of the asset manager to figure out how it how it kind of functions right and again if you're running out of memory and this isn't used it could start cleaning out assets anyway lots of obviously very complicated systems we could set up for this but uh you know at a very at the very least i would do that but to fix the memory leak and to keep this code as simple as it is now i guess we would even though it hurts me i guess we would just um keep loading it every frame every time we write this we write text but at least close it you know um so that's kind of yeah looking through the rest of this code i don't see any other major problems so we're drawing a bunch of lines here you know you're passing vectors by value which you definitely shouldn't be so just keep in mind that vectors obviously if vectors get copied then they will actually do a heap allocation to contain all the memory so it's not really about memory per se it's more about performance because dealing with large amount of memory or allocating memory and then freeing it and stuff like that that's obviously going to take up cpu cycles so we can kind of avoid that by just simply referencing this instead especially if it's kind of well in this case obviously it's just read only just reading the data from there you just need access to the vector you don't need to actually create a whole new copy of the vector which is what your code is currently doing so just remember basically anything that is sizable try and pass it by const reference now this doesn't apply for things like vector 2 even though it is two doubles which is 16 bytes like that's not really that big of a deal 16 bytes to copy no worries the problem isn't really the memory that we're copying it's the fact that copying a vector like the vector copy constructor you know needs to create a new vector and creating a new vector means we have to do a heap allocation to contain all of the elements within the vector so it's more or less the code within the copy constructor and that whole kind of you know creating a vector situation it's the code there that's doing the allocation that's the bad thing not the fact that the vector itself takes up space right kind of like a vector2 would 16 bytes but it's okay um same with like string again you know for stuff like text the thing with this is that you know if we're reading this text like what you're doing here is copying the string which again is not really ideal because if you take a look at this right i mean let's take let's use this as a little case study um and i think we i mean i really want to fix that other bug but yeah these videos man these videos are just so long anyway uh what am i even looking for um let's actually okay va find references uh okay so write text so for example you know we're writing text over here you know what is this um okay we have like press escape for menu and then yeah we've got like a bunch of is this actual oh subtext is what we're writing yeah so subtext is i don't know what this is to be honest but anyway subtext we write like for example press escape for menu right this gets written immediately meaning that like you know in this function in the scope of this function this is all single threaded right you don't need to worry about actually copying the string in the sense that the string's lifetime is tied to this function right which means that we have to be done with it or copy it by the time this function is terminated because this memory will in fact be freed right so you have to kind of keep that lifetime in your mind so that being said we come over here and we go to rendertex solid now what does this do well this takes in a const chart pointer and it obviously uses it immediately internally right it doesn't reference it and do something it will actually draw it right i don't know exactly how sdl works but my point is that it will take that text probably copy it into some buffer if it needs to and deal with it meaning that in basically here you can free the string memory right as soon as you've given it into here and this is how most kind of renderers work right it has taken it it's done what it needs with it it's copied if required and then that means that again it's not your responsibility to keep this text alive uh you know for like what until the next frame or something no right you can be done with it immediately so what that means is that we don't need to make a copy of it or anything like that right we can uh just kind of use this existing memory that i keep losing everything um we can just use this existing memory that we created here right for this press escape for menu why have this duplicated in memory we don't need that we can just have it in memory once which is here and in fact this is probably going to be like a cons char anyway but we can have it in memory in a particular place and then that's it so what you should do instead and of course like in this case it is a conch char but if you're you know maybe using like um i don't know string stream or something to like you know add in like for example you were displaying the score so obviously that's kind of a dynamically generated string it's not just in read-only memory and then in that case obviously uh you know you would be kind of copying it but again if you know the lifetime instead of doing that it'd be much better to use um to either pass it by const reference or to use like string view right using simple plus 17. so you can just use string view which basically just means give me a pointer to like the string um and to its end i guess uh and then that's kind of it right so again you're not copying the string you're just using the string that you've already created over here right because then then you get into this habit of like okay but if i create functions i have to pass stuff into those functions which means that it's almost like the more functions you make the slower your program becomes because every time you call a function you're copying everything into that function that already exists in the parent kind of calling function and that of course is not something that we want to or need to do right so just keep keeping keeping keep an eye on that and again coming from something like java or c sharp it's kind of easy to just not realize this because if you have a string or you have um this vector right which would be like a list in java or c sharp like objects in those languages are like pointers essentially right they're not actual kind of stack allocated objects that you can copy if you were to write this exact code in c sharp and it might be something like public void draw lines and then here maybe you have like some kind of list you know how would you write this in c sharp well it would basically look like this right so by doing this though again list is basically being passed as like a pointer or a reference not like this if you were to kind of compare it to the c plus plus equivalent and that's another thing that people coming from java c don't realize in c plus that like you know you don't have to use the new keyword everywhere and um you know classes like that don't really translate it's kind of like a struct i guess in c sharps structs and c sharp are also kind of value based not reference based but anyway the point being that to convert this properly you know you would actually have to add it and add in that pointer but instead of doing that in c plus plus we can just add a reference um and again i guess point of reference same same kind of idea but the point is this now means that it's it will effectively work like it would in c sharp or java right if you were to pass in a list into a function in the sense that it doesn't copy the list it just references it and that's how you achieve that in c plus so that's another really important thing to to get your head around if you're new to c plus plus okay so the final thing that i want to take a look at is this weird mouse lag thing right and again there's so much we could take a look at here honestly i could probably sit here all day and just go through this line by line um but in the interest of time let's see what's causing that because it's very weird now i'm going to look at the game loop because strangely enough again i don't think it's actually a frame drop because if it was a frame drop then we'd probably see the cpu usage go up right i think these frame times are fine so let's go ahead and look at the actual loop right because that's kind of the first this is the entry point essentially into every frame so we okay so the first thing we do is we check for events um this is fine so if not should quit i don't know if you should necessarily handle this again i guess if there's a quit event then that will come up maybe but it doesn't look like it would so i don't know if we necessarily need to like check that but we pull all of the events we retrieve them then we go handle event which what does that do okay that sees what it all right okay so one of the events could be a quit event which then means you don't have to keep processing events okay fair enough um and then we yeah we can do all this stuff mouse button again handle mouse move i don't think this uh i guess it depends what like playable level i think is the actual level and what was that handle mouse move or something okay so if it's the left mouse button then uh we draw like this trail which uh yeah looks looks like it's just a bunch of lines um okay sure yeah don't really is drawing trails surely that doesn't stop the balls from moving um it's drawing trail that seems very local um okay sure i guess it's a no it's private anyway um so yeah what could it be why is the time changed inside the event okay so we get the last equals now and then we get that and then we calculate now minus last but that's going to be like zero so if we go through events and we update the time to the current time then we do this one little thing which clearly takes no time at all then we get now again so last is equal to this and then we get the difference this is going to be like zero this is probably why because we we're effectively doing this i mean if you wanna if you want it even more clearer we're effectively just doing this right so the point is obviously with delta time to try and figure it between frames so we want to see how much time has passed through the entire frame including like presenting the rendering including waiting for vsync possibly so i think this is the culprit actually because if a delta time is zero nothing will move um okay you've got a max time step that's good and you looks like you're updating more if you need to this is all pretty good sure um yeah let's try removing this that's my initial suspicion because this is just evaluating to zero and obviously if we keep getting events yeah there we go that's fixed if we keep getting events then we're not going to move anywhere because we'll always be like literally you'll only get a proper delta time if there are zero events that's how it's currently set up and yeah no memory leak okay cool i haven't checked out all the levels um i don't know if we will but yeah overall pretty cool game um i think that you know for someone who's coming from other languages you've done a pretty decent job in using c plus plus obviously there's a lot of things i would personally change but it's hard to differentiate between stuff that's actually you know objectively good you know i've tried to point out stuff like that like not copying strings or vectors making sure that you pass by reference um uh when needed because i like you do sometimes with these pointers but that's because the objects are like that um obviously being you know creating a cache for your text stuff and possibly an asset manager in the future but overall like yeah this looks like pretty decent code um you clearly seem to more or less know what you're doing again i'll just assume the memory leak was like a bug um and that just something that you accidentally missed although you have done it in two different functions no actually sorry it wasn't a leak here but again opening like i just have a problem with opening this file like every frame potentially reading it rasterizing it you know well actually i don't know if it gets gets rasterized here it might get right it gets it might actually rasterize each character as it needs it when you actually try and render a particular text string that's what i'm assuming it does i don't think it just immediately sets up like all of the ascii characters or some kind of character set into an atlas immediately but who knows anyway the other thing that i like is that for example your level select menu actually uses um the directory iterator from sql 17 to find out which levels we have right so it's all very dynamic if the file exists there then you can load it basically which is pretty cool i mean if we went to our levels and we duplicated this and we called it churno then i'm assuming we would get a level called chono and there it is we have a level called chono which is the same as whatever level i copied right so that's pretty cool a little bit more dynamic i like that you also seem to have all of these deleters um sdl window deleter what's the point of all of this um because you did mention you wanted me to take a look at like stl lifetime stuff but what's the ah i see okay yeah okay that's interesting so you've put stl window and sdl renderer into a unique point and specified a custom deleter right because obviously sdl is all like c based and you have to explicitly essentially have a destructor that calls this um i guess that's okay like at the end of the day i probably maybe i'm just a little old-school but i probably wouldn't do this um and at the very least you could probably you know this these files are like a few lines of code so you could just combine them into one file called sdl deleters and then just have them all there that would be a little bit more organized and we'll just reduce the amount of files you have to browse through but i would probably just have like an sdl init function an sdl shutdown function like a renderer in it render a shutdown or whatever application in it application shutdown manually call all of them that for me that just keeps it clean because like with a lot of these things like you have to be explicit with when they get deleted right or shut down right so the order matters and if you just have unique pointers that are in a stack that can be obviously a little bit less explicit as to like the actual order anyway though i hope you guys enjoyed this video let me know what your thoughts were this was a very long one and this was a hard one for me because there was a lot of stuff to point out and um you know i mean i didn't really get into the game logic so much i more or less talked about the code in the code style and all the various problems it had i hope that me solving some of these issues like the memory leak and showing how i approached that without ever having seen this code before i hope that kind of stuff is helpful to you if you run into similar problems and also maybe will prevent you kind of prophylactically from writing that sort of code let me know what you thought in the comment section below if you enjoyed the video please don't forget to hit the like button let me know what kind of content you want to see next in the code review don't forget that you can email me at channelreview gmail.com it will be in the description below with your code a little bit of an explanation as to um you know whether you want to be anonymous or not and what the project is and hopefully i'll see you in the next episode of the code review series don't forget to check out skillshare using the link in description below and i'll see you guys next time goodbye [Music] you

Channel: The Cherno

Views: 40,663

Rating: 4.9711342 out of 5

Keywords: thecherno, thechernoproject, cherno, c++, programming, gamedev, game development, learn c++, c++ tutorial, code review, java, c#, python, javascript, learning c++, how to learn c++, moving to c++, cpp, c++ code review, cherno code review

Id: gkEHofFbYyU

Length: 47min 46sec (2866 seconds)

Published: Fri Oct 01 2021