• Advertisement

Recommended Posts

I'm a C++ newbie, and I heard a lot that the best way to learn how to code is actually code. So I tried. The project is still in progress so please don't be embarrased. At least I think it's tiny enough to be read quickly.

Here it is

https://github.com/FennecFix/Delirium/tree/master/src

It written in C++ with use of Oxygine framework. Please don't mind the commit comments, it wasn't intended to show in public.

I really want to aks you to point out any flaws of my code

Share this post


Link to post
Share on other sites
Advertisement

There's too much code there for me to find time to review it, unfortunately. It looks good and clean for the most part. In a couple of places I think there are too many getters and setters which are probably unnecessary. I also probably wouldn't personally choose a full Model / View / Controller approach for a game, but if it works for you, continue. I would also like to see more comments explaining what the code is doing, and fewer comments that tell me nothing (e.g. "//back bottom" right above 2 lines where 'backBottom' is obvious in the code).

Share this post


Link to post
Share on other sites

Thanks for your reply!

Quote

I think there are too many getters and setters which are probably unnecessary

Should I make it just public?

 

1 hour ago, Kylotan said:

I also probably wouldn't personally choose a full Model / View / Controller approach for a game, but if it works for you, continue.

Just felt natural for me. Anyway, can you suggest what approach would you prefer?

 

1 hour ago, Kylotan said:

I would also like to see more comments explaining what the code is doing, and fewer comments that tell me nothing (e.g. "//back bottom" right above 2 lines where 'backBottom' is obvious in the code).

Yeah, sorry for dumb comments.

I thought my code is kinda self-explanatory, so I didn't add much comments.

I know that code review is time consuming, so maybe I just asked about it in wrong place? Can you suggest where I should go?

Share this post


Link to post
Share on other sites

Usually if an object has a lot of getters and setters it implies that the interface to it is not well designed. Usually these can be removed and replaced with methods that do a better job of explaining the intent of the change or the query being performed.

If MVC feels natural to you, keep it. It looks very 'clean', once you get used to it. It can lead to functionality being split across 3x as many files as usual, but that may be a price worth paying.

Code is almost never self-explanatory. It's usually possible for a programmer to read a whole function and, if it's written well, to understand what it does. But what if I don't have time to read the whole function? Or what if there's a hidden bug, but we can't tell it's a bug because there's no comment that explains the true intention? And what if I want to understand when a certain function will be called?

I don't know if there is anywhere where people will be willing to review large amounts of code for free. There is codereview.stackexchange.com but (a) they're not game developers, and (b) they don't usually post this much code.

A few extra thoughts while I'm here:

  • I see some bi-directional dependencies - such as WorldModel creating UnitModel and holding a reference to it, but that UnitModel also holds a reference back to the WorldModel. Often this is a sign of bad design because it makes the 2 classes tightly coupled. (But sometimes the alternative is worse.) And are you sure you're not creating memory leaks with these circular references? If the reference counts never hit zero due to these mutual references, then the objects won't get freed.
  • You probably don't want to rely on vector length to indicate a collision or not, as in WorldModel::update. You already have a 'check' version of the function so it's not clear why you're doing this.
  • Why keep calling .get() on shared_ptrs before using the value? The whole point of them is that you can use them the same way that you'd use a normal pointer.
  • There's probably a more elegant way to remove your dead projectiles from the list. Maybe use a lambda with the erase/remove idiom (see example: http://en.cppreference.com/w/cpp/algorithm/remove)
  • CollisionSolverAABBxAABB has some duplicated code. You want to avoid this if possible. (i.e. Don't Repeat Yourself.)
  • _faceDiretion is spelled wrongly.
  • I'm not a fan of a leading underscore to denote a member. It looks a lot like the syntax reserved for compiler implementation identifiers, and it's an extra thing to type when using code completion or Intellisense. I've seen codebases with trailing underscores, or `m_` prefixes, but generally I just don't use any special markup for these. It sometimes means I have to be more creative with function arguments, but it makes the rest of the code a lot cleaner.

Share this post


Link to post
Share on other sites
Quote

UnitModel also holds a reference back to the WorldModel

Since WorldModel keeps vector of UnitModel, it seems to me like the only way to get a list of units in sight of one unit. Or if unit needs to fire a bullet, it's needed to put this bullet in the world. somehow. Just don't know how else do I do that.

 

1 hour ago, Kylotan said:

And are you sure you're not creating memory leaks with these circular reference

 

1 hour ago, Kylotan said:

Why keep calling .get() on shared_ptrs before using the value? The whole point of them is that you can use them the same way that you'd use a normal pointer.

As far as I know .get() not increasing reference count, that's why I pass raw pointers. Is it a bad practice?

Edited by ????????? ????????

Share this post


Link to post
Share on other sites
1 hour ago, ????????? ???????? said:

As far as I know .get() not increasing reference count, that's why I pass raw pointers. Is it a bad practice?

Depends if the callee needs to own the pointer and the pointer becomes dangling.

Take a look at this answer on StackOverflow: https://stackoverflow.com/a/8706254/1731200

The answer explains when to use smart and raw pointers.

Share this post


Link to post
Share on other sites

 

15 hours ago, ????????? ???????? said:

Since WorldModel keeps vector of UnitModel, it seems to me like the only way to get a list of units in sight of one unit.

It's often possible to fix bidirectional references by having the 'owner' (in this case, the WorldModel) pass in some sort of callback or interface so that the UnitModel can query for this. Sometimes, it's okay to have the owner implement the interface - e.g. WorldModel inherits from a new 'UnitFinder' interface), meaning the physical relationship is still bidirectional (WorldModel <--> UnitModel) but the logical relationship is different (WorldModel -> UnitModel, UnitModel -> UnitFinder).

Here's some discussion on why such bidirectional references are not perfect: https://refactoring.guru/change-bidirectional-association-to-unidirectional

If this doesn't make sense to you yet, don't worry about it for now.

15 hours ago, ????????? ???????? said:

As far as I know .get() not increasing reference count, that's why I pass raw pointers. Is it a bad practice?

There is almost no good reason for passing raw pointers around if you have smart pointers available. If you want to pass the object to be used temporarily and don't feel like a shared_ptr is necessary, you should dereference the shared pointer and pass that object by reference.

Share this post


Link to post
Share on other sites
3 hours ago, Kylotan said:

you should dereference the shared pointer and pass that object by reference

It could be nullptr. Furthermore, you could cache pointers in a collection (to avoid lots of unnecessary ref count increases and thus atomic adds per frame) which is not possible for references unless you use something like the std::reference_wrapper.

Share this post


Link to post
Share on other sites

I don't think the ref-counting cost is relevant in this situation.

As for nulls, he's not checking for those anyway. Besides, if you're not deliberately doing something dangerous then it's hard to end up with a null in a shared_ptr.

Share this post


Link to post
Share on other sites
23 hours ago, ????????? ???????? said:

Since WorldModel keeps vector of UnitModel, it seems to me like the only way to get a list of units in sight of one unit. Or if unit needs to fire a bullet, it's needed to put this bullet in the world. somehow. Just don't know how else do I do that.

A common way to do this is with some kind of event system/queue. For simplicity sake you could also just add a small queue to your units that the world iterates over and processes, decoupling the unit from the world entirely.

4 hours ago, matt77hias said:

It could be nullptr. Furthermore, you could cache pointers in a collection (to avoid lots of unnecessary ref count increases and thus atomic adds per frame) which is not possible for references unless you use something like the std::reference_wrapper.

 

3 hours ago, Kylotan said:

As for nulls, he's not checking for those anyway. Besides, if you're not deliberately doing something dangerous then it's hard to end up with a null in a shared_ptr.

Most of the time I don't see a need for shared_ptr, and I've been burned before by people misusing/overusing them. In many of those situations unique_ptr was a better choice.

I don't believe shared_ptr should even be used unless ownership needs to be shared. If not, use unique_ptr and pass around references or raw pointers if the situation calls for it.

I suppose you could use weak_ptr, but I personally find the usage there irritating. It also doesn't get around the fact that ownership of the original data can still be shared.

Edited by Styves

Share this post


Link to post
Share on other sites
1 minute ago, Styves said:

I don't believe shared_ptr should even be used unless ownership needs to be shared. If not, use unique_ptr and pass around references or raw pointers if the situation calls for it.

Exactly my point: https://stackoverflow.com/a/8706254/1731200

Nothing wrong with using raw pointers.

Share this post


Link to post
Share on other sites

I do understand the argument over not overusing shared_ptr, but in other languages like C# or Python, pretty much everything is a shared_ptr and those languages are productive to work with. As such, I'm happy recommending its general use as a starting point, with the other options being there to allow optimisation.

Share this post


Link to post
Share on other sites
10 hours ago, Kylotan said:

I do understand the argument over not overusing shared_ptr, but in other languages like C# or Python, pretty much everything is a shared_ptr and those languages are productive to work with. As such, I'm happy recommending its general use as a starting point, with the other options being there to allow optimisation.

C++ isn't designed to deal with the consequences of shared-only ownership semantics like C# and Python are. Imagine what would happen if you introduced a circular reference (either intentionally or otherwise) while taking this advice. I've seen it take days for even seasoned engineers to find the cycles, let alone someone new to the language.

There are plenty of concepts I consider safe to simplify for newcomers to C++, but dynamic memory and ownership semantics just aren't among them. If you're going to play with fire, you have to know what you're doing.

Share this post


Link to post
Share on other sites

If you introduce a circular reference with shared_ptr, you get a memory leak. This is often harmless in small applications.

If you mishandle raw pointers, your program crashes stone dead, and if you debug it, often it's not obvious why it's crashing. This can be very discouraging to a newbie to C++.

I think it's important to understand the limitations of reference counting - which is why I did highlight the circular reference issue earlier in this thread - but I still think shared_ptr as default is a reasonable starting point.

Share this post


Link to post
Share on other sites

I look at it from the point of view of someone who has to work with code written by others on a day-to-day basis (as do many of us), and ultimately share the burden of any problems or issues that arise from it. And not just code written by other team members, but also code written by my past self. Some of the trickiest bugs I've encountered were caused by code written with an incomplete or incorrect understanding of core concepts -- ownership being among the most common. And as you've pointed out, these can be insidious bugs that hide in code for a long time before rearing their ugly heads at the worst possible times.

So perhaps I'm just being cynical, but I don't mind if someone is discouraged from learning C++. Do we really have any control over that anyway? I'd rather make sure that if they ever do reach the point of working with others, they're not adding to the team's burden, increasing technical debt, or otherwise causing problems that could have been avoided had they a better understanding of what they were doing.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now


  • Advertisement
  • Advertisement
  • Popular Tags

  • Advertisement
  • Popular Now

  • Similar Content

    • By sergio2k18
      Hi all
      this is my first post on this forum.
      First of all i want to say you that i've searched many posts on this forum about this specific argument, without success, so i write another one....
      Im a beginner.
      I want use GPU geometry clipmaps algorithm to visualize virtual inifinte terrains. 
      I already tried to use vertex texture fetch with a single sampler2D with success.
       
      Readed many papers about the argument and all speak about the fact that EVERY level of a geometry clipmap, has its own texture. What means this exactly? i have to 
      upload on graphic card a sampler2DArray?
      With a single sampler2D is conceptually simple. Creating a vbo and ibo on cpu (the vbo contains only the positions on X-Z plane, not the heights)
      and upload on GPU the texture containing the elevations. In vertex shader i sample, for every vertex, the relative height to te uv coordinate.
      But i can't imagine how can i reproduce various 2d footprint for every level of the clipmap. The only way i can imagine is follow:
      Upload the finer texture on GPU (entire heightmap). Create on CPU, and for each level of clipmap, the 2D footprints of entire clipmap.
      So in CPU i create all clipmap levels in terms of X-Z plane. In vertex shader sampling these values is simple using vertex texture fetch.
      So, how can i to sample a sampler2DArray in vertex shader, instead of upload a sampler2D of entire clipmap?
       
       
      Sorry for my VERY bad english, i hope i have been clear.
       
    • By mangine
      Hello. I am developing a civ 6 clone set in space and I have a few issues. I am using Lua for the logic and UI of the game and c++ directx 12 for the graphics. I need a way to send information between Lua and c++ occasionally and was wondering what is the best and most flexible (and hopefully fast) way to do this. Don't forget that I also need to send things from c++ back to Lua. I know of a lua extension called "LuaBridge" on github but it is a little old and I am worried that it will not work with directx 12. Has anybody done something similar and knows a good method of sending data back and forth? I am aware that Lua is used more and more in the industry and surely plenty of AAA game programmers know the answer to this. I want a good solution that will hopefully still be viable code in a couple of years...
    • By owenjr
      Hi there.
      I'm pretty new to this and I don't know if it has been asked before, but here I go.
      I'm developing a game using SFML and C++.
      I would like to use the "Tiled" tool to load maps into my game but I don't actually find any tutorial or guide on how to exaclty use it (I know that I have to read an XML file and stuff). I just step into diverse projects that make all a mess. 
      Anyone knows where can I find good information to make my map loader by myself?
      Thanks in advantage!!
    • By MHG OstryTM
      Hello guys,
      I've released my game for the first time. I'm very excited about it and I hope you'll enjoy the game - Beer Ranger. It's a retro-like puzzle-platfromer which makes you think a lot or die trying. You have a squad of skilled dwarfs with special powers and your goal is tasty beer. There is a lot of traps as well as many solutions how to endure them - it is up to your choice how to complete the level! 
      Link to the project: Project site
      Link to the Steam site with video: Beer Ranger
      Have fun and please write feedback if you feel up to.
      Some screens: 




    • By francoisdiy
      So I wrote a programming language called C-Lesh to program games for my game maker Platformisis. It is a scripting language which tiles into the JavaScript game engine via a memory mapper using memory mapped I/O. Currently, I am porting the language as a standalone interpreter to be able to run on the PC and possibly other devices excluding the phone. The interpreter is being written in C++ so for those of you who are C++ fans you can see the different components implemented. Some background of the language and how to program in C-Lesh can be found here:

      http://www.codeloader.net/readme.html
      As I program this thing I will post code from different components and explain.
  • Advertisement