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

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


  • Announcements

  • Forum Statistics

    • Total Topics
      628378
    • Total Posts
      2982334
  • Similar Content

    • By Kazuma506
      I am trying to recreate the combat system in the game Life is Feudal but make it more complex. The fighting system works by taking in the direction of the mouse movement and if you press the left click it will swing in that direction, though stab, overhead, left (up, down, left right) and right are the only swings that you can do. If you wanted to you could also hold the swing by holding left click so you are able to swing at the perfect moment in the battle. I want to change this so add in more swing directions but I also want to code this from scratch in Unreal. Can anyone give me any pointers or maybe a few snippets of code that work in Unreal that could help me start to implement this type of system?
       
       
    • By rXpSwiss
      Hello,
      I am sending compressed json data from the UE4 client to a C++ server made with boost.
      I am using ZLib to compress and decompress all json but it doesn't work. I am now encoding it in base64 to avoid some issues but that doesn't change a thing.
      I currently stopped trying to send the data and I am writing it in a file from the client and trying to read the file and decompress on the server side.
      When the server is trying to decompress it I get an error from ZLib : zlib error: iostream error
      My question is the following : Did anyone manage to compress and decompress data between a UE4 client and a C++ server ?
      I cannot really configure anything on the server side (because boost has its ZLib compressor) and I don't know what is wrong with the decompression.
      Any idea ?
      rXp
    • By noodleBowl
      I was wondering if someone could explain this to me
      I'm working on using the windows WIC apis to load in textures for DirectX 11. I see that sometimes the WIC Pixel Formats do not directly match a DXGI Format that is used in DirectX. I see that in cases like this the original WIC Pixel Format is converted into a WIC Pixel Format that does directly match a DXGI Format. And doing this conversion is easy, but I do not understand the reason behind 2 of the WIC Pixel Formats that are converted based on Microsoft's guide
      I was wondering if someone could tell me why Microsoft's guide on this topic says that GUID_WICPixelFormat40bppCMYKAlpha should be converted into GUID_WICPixelFormat64bppRGBA and why GUID_WICPixelFormat80bppCMYKAlpha should be converted into GUID_WICPixelFormat64bppRGBA
      In one case I would think that: 
      GUID_WICPixelFormat40bppCMYKAlpha would convert to GUID_WICPixelFormat32bppRGBA and that GUID_WICPixelFormat80bppCMYKAlpha would convert to GUID_WICPixelFormat64bppRGBA, because the black channel (k) values would get readded / "swallowed" into into the CMY channels
      In the second case I would think that:
      GUID_WICPixelFormat40bppCMYKAlpha would convert to GUID_WICPixelFormat64bppRGBA and that GUID_WICPixelFormat80bppCMYKAlpha would convert to GUID_WICPixelFormat128bppRGBA, because the black channel (k) bits would get redistributed amongst the remaining 4 channels (CYMA) and those "new bits" added to those channels would fit in the GUID_WICPixelFormat64bppRGBA and GUID_WICPixelFormat128bppRGBA formats. But also seeing as there is no GUID_WICPixelFormat128bppRGBA format this case is kind of null and void
      I basically do not understand why Microsoft says GUID_WICPixelFormat40bppCMYKAlpha and GUID_WICPixelFormat80bppCMYKAlpha should convert to GUID_WICPixelFormat64bppRGBA in the end
       
    • By HD86
      As far as I know, the size of XMMATRIX must be 64 bytes, which is way too big to be returned by a function. However, DirectXMath functions do return this struct. I suppose this has something to do with the SIMD optimization. Should I return this huge struct from my own functions or should I pass it by a reference or pointer?
      This question will look silly to you if you know how SIMD works, but I don't.
    • By pristondev
      Hey, Im using directx allocate hierarchy from dx9 to use a skinned mesh system.
      one mesh will be only the skeleton with all animations others meshes will be armor, head etc, already skinned with skeleton above. No animation, idle position with skin, thats all I want to use the animation from skeleton to other meshes, so this way I can customize character with different head, armor etc. What I was thinking its copy bone matrices from skeleton mesh to others meshes, but Im a bit confused yet what way I can do this.
       
      Thanks.
  • Popular Now