Minecraft Clone in C++ // Code Review

Chapters:
----------------
0:00 - What we're looking at today
6:55 - Playing the game
8:55 - Project structure and initial thoughts
10:30 - Local static singleton
14:10 - How to organize a class (my style)
17:43 - The math
19:22 - Ray casting and how I would change it
23:55 - Designing code structure
28:08 - Other notes
29:23 - Some weirdness and how to write clear code
35:22 - Vertex packing and shaders

This video is sponsored by Brilliant.

#CodeReview

[not_herobrine]

"the only design pattern a programmer needs to know is ctrl + c and ctrl + v" -sun tzu, the art of code

[lukefidalgo]

I think every programmer that's interested in making games from scratch will always go through that phase where they want to make a Minecraft clone lol

[toffeethedev]

love higher-level design talks like this, optimisations are usually specific to the 1 program, but knowledge about code architecture will never stop being useful

[bloodgain]

Additional comments (and disagreements with Cherno's comments):
- For a better understanding of the Application instantiation argument here, just look up why Singletons are considered a bad practice. If you're going to use a Singleton in C++, though, _always_ use the Meyers Singleton (thread safe), which you did. On this count, Cherno is wrong and going against widely-regarded best practice. He is right that it's subject to the Destruction Order Fiasco, but if you insist on a singleton, you live with that. Just don't have singletons that depend on other singletons -- as if singleton wasn't a bad enough design! There is another pattern called Leaky Singleton that avoids destruction entirely, but one, if you need that you have a bad design, and two, if you're going to go through the trouble of learning how make a good, thread safe, leaky singleton, you'd be better off spending the time re-architecting to get the singleton(s) out of your design.
- While it's an architecture argument, I probably agree with Cherno's critique of your Ray class. However, passing the World into the function like that? That's _exactly_ how you avoid Singletons **and** tightly-coupled code. It's called Dependency Injection, one method of Inversion of Control -- aka coding to interfaces, not objects. Good instinct, if not the best use of it in this case. Still, way better than making World a singleton!
- Cherno's argument against using explicit is simply wrong. You absolutely should mark all single-argument constructors as explicit. Not doing so can result in accidental implicit conversion, which adds runtime overhead. All such conversions should be made explicitly, usually by calling static_cast or dynamic_cast. Again, this is _widely_ considered best practice.
- Why is it called main? Why not? This is an application, not a library. It's probably the IDE's default, and defaults are de facto patterns. Plus, it's just a learning project, and it could always be renamed later. Code reviews should not be nit-picking exercises.
- Class layout is a little bit of a style nit-pick, but in this case, I actually agree. This is the standard layout that other developers expect to find. If you declare it as a struct (public access by default), this signals something a little different to another developer that this is more datatype than functionality, will probably be entirely public, and you can leave off the "public" declaration. Think of these as de facto standards.
- Cherno doesn't call you out for not separating your code from your declarations, but I will. If you're going to enclose all your class's functionality in one file, it should be a header (.h) file. But still, separate your code from the declaration, either as "inline"d functions below the class declaration or in a code (.cpp) file. The _possible_ exception is simple one-liners like getters/setters that are only an assignment/return, but separate is better even then. This is another de facto standard, and most style guides enforce it.
- If you have a destructor, either write the copy and move constructors and assignment operators, or remove them with "= delete". If you write any of these, you should write all of them. This is called the Rule of Five (formerly Rule of Three before move semantics were added in C++11).
- Your "chunks" map is duplicated. Maybe that actually fits your design well, as long as you're not carrying 2 full duplicates of all of them in memory, but it does mean that *the map type is declared in more than one place.* Abstract that out. At minimum, use a typedef, but if "chunks" has responsibilities beyond what unordered_map provides, then it should be its own type with functions. A clue here would be if you're repeating code to operate on chunks in multiple places.
- Overall, though, great job. Some of it is pretty clever, actually. Maybe read a style guide or two and check out some C++ best practices guides. Scott Meyer's "Effective" series of books is highly recommended, but there are some good free guidelines and talks out there, too.

[andrewcranston]

When doing code reviews, its always good to keep personal 'feeling' or code style debates out. "I just don't like this" isn't a good reason, and doesn't go far in explaining why someone should avoid using singleton for the top level Application class. You could instead go the 'singleton is hard to test' route, and explain how you'd eventually want to write unit tests and instantiate the application inside a test harness.. if its designed to be statically instantiated that becomes hard to do.

[vladimirkraus]

"explicit" is your friend. Unwanted or unexpected implicit conversions can cause a lot of troubles and hard to find bugs. I always use "explicit" unless I am really 100% sure that I want an implicit conversion for that type. Explicit conversion requires more writing but makes the code much more readable and simpler to reason about. It makes the code more type safe and less prone to errors.

[jumpingjoy]

The reason that there is a lot of "explicit" in the code is that the programmer is most likely using clion, which includes clang-tidy, which alerts you about single argument constructors. it is also why you see [[nodiscard]] aswell

[migueldejesustavares]

You should 100% make your own Minecraft Clone tutorial series. It would be such a good learning resource.

[propov]

16:16
C++ CoreGuidelines. "C.46: By default, declare single-argument constructors explicit
Reason To avoid unintended conversions."

[IONATVS]

I believe Minecraft Bedrock Edition (ie the modern version of Minecraft Pocket Edition, which is available on Mobile, Consoles, and Windows PCs) has a C++ codebase, as opposed to Java Edition (the Original, available on Windows, Mac, and Linux)’s use of Java (obviously, it’s in the modern name). Maybe it was C#, but in any case, Bedrock is already an entire rewrite of Minecraft in an entirely different language.

[sarahkatherine]

30:46 I'm sorry if this is not Isti's case.

This "MovementDirection" trick seems to be used by a lot of previous-generation teacher and programmer (or at least in my country).
The idea is, instead of writing x += 0; y += 1; to move upward, they will create a "movement direction array" like this:
dirX = {0, -1, 0, 1, -1, 0, 1, -1, 0, 1};
dirY = {0, -1, -1, -1, 0, 0, 0, 1, 1, 1};
Now to move upward, they will do x += dirX[8]; y += dirY[8]; because Num8 is up on your numpad (you are at Num5).
They said it will saves a lot of if-else for all 8 directions, and makes it easier to tract the direction by looking at the numpad and array index.

[zxuiji]

16:07, another reason to put public stuff 1st is to reduce ABI issues when you change something in the private section, if the public stuff is 1st then it will remain in the same ABI spot so long as nothing changes in the public section (or what it inherits from)

[ShinQdan]

24:20 It's funny the actual Minecraft is written this way in Java as far as I remember - loads of passing the World object down to entities

[h.hristov]

Hi Cherno, could you create your own series on design patterns? I wonder what your take would be on them. Recommendations / pitfalls.

[beastleend]

I actually do this with the explicit too if the constructor only has one parameter, because I think it's a better design to have everything explicit by default. This is exactly how you actually make every reference const at first, unless you already know you want to mutate it.

[DipsAndPushups]

At 24:50 Yes, PLEASE! Those kinds of questions of OOP, questions of design, what class should do what is what I want to know so bad. I would like to watch videos on that topic in fact this very discussion about what the Ray and other objects should know about the world and which class should take care of collision detections is something that I am really interested to know. It is also interesting that you can build projects as big as Minecraft clones in OpenGL despite the fact that your design decisions are bad.

[rmt]

That's so exciting! Just started planning out my own Game Engine, and you're one of my recently found inspirations! Been watching a lot of your videos sense. To know that I can send it in for you to review, that's super exciting!!! Deffinately have my heart set on that now!

Btw, you should totally make your own minecraft clone! Would love to see one done with hazel!!!

[joelincz]

It's inspiring to see what people are creating at the same time super insightful to hear an experienced person comment on it. Thank you to both!

[victornpb]

while I agree with most of the criticisms, I find that a lot in this video comes down to personal preference, and the author didn't do anything that is specifically bad. I think it's important to separate what are "do's and don’ts" from "there's this other way some people prefer", this discrimination is something very important I learned in code review etiquette.

[DavidSpry]

I suspect he's using Clang-Tidy, which is fussy about the use of `explicit` for single-parameter constructors.