Vega-strike-engine-source: Use smart pointers where applicable

[stephengtuggy]

Personally, I think the C++11 standard is old enough (8 years old!) that we can begin using it. And it comes with some great added features! Like the smart pointer types std::unique_ptr, std::shared_ptr, and std::weak_ptr: https://en.cppreference.com/w/cpp/memory.

If we start using these smart pointer types wherever possible, a lot of Vega Strike's memory management issues could probably be solved.

What do you guys think?

[klaussfreire]

I agree with the sentiment, but a lot of distros still come with gcc versions that don't enable c++11 support by default. No big deal, just a heads-up.

FTR, we're already using a few features from c++11 through tr1 and boost, so it's not like we're missing on them.

[stephengtuggy]

Looks like boost includes a boost::shared_ptr type, which internally maps to c++11 std::shared_ptr when that is available, or to a different implementation otherwise. I would be happy to use boost::shared_ptr instead.

In fact, someone else already started using it before me, in at least one or two places in the code.

[stephengtuggy]

Perhaps even more suited to our needs: boost::intrusive_ptr. The name sounds a little weird, but the concept actually fits what Vega Strike already does: keep an internal ref count within each object instance (of Unit, for example).

Potential advantages of using boost::intrusive_ptr over what we already have:

• It's written and reviewed by domain experts, and thus very likely to be correct
• It's an established pattern as part of a well-known and respected library that we already use to some degree
• It is (or can be) thread-safe, which the current implementation... uh... isn't entirely ;-)

[LifWirser]

In theroy I agree
now look at what else could be afected

• Vegastrike shares data referenced from pointers back and forth between c++, Python> GL(whichever), sound and various network functions.
  As I am new to contributing I want to know that all sides are looking at what they are supposed to
• from my perspective a through "Audit" of th VS codebase needs to be performed. Basically for this topic all pointers need to be idenified bot of origin(s) and users, along what tools are needed to test/verify proposed changes

Making this a Goal sounds like a result of properly auditing the codebase.

[BenjamenMeyer]

See #15 for the audit proposal by @LifWirser

[stephengtuggy]

In theroy I agree
now look at what else could be afected

* Vegastrike shares data referenced from pointers back and forth between c++, Python> GL(whichever), sound and various network functions.
  As I am new to contributing I want to know that all sides are looking at what they are supposed to

* from my perspective a through "Audit" of th VS codebase needs to be performed. Basically for this topic all pointers need to be idenified bot of origin(s) and users, along what tools are needed to test/verify proposed changes

Making this a Goal sounds like a result of properly auditing the codebase.

@LifWirser I absolutely agree. I started going down this path on a branch in my own fork of Vega-Strike-Engine-Source; in the process, I tried to take into account all these different parts of the game where these pointers might be used. With most types of smart pointers, there are ways to extract regular pointers from them when needed (although these may limit how useful the smart pointers actually end up being).

In the end, there were a HUGE number of changes that needed to be made, and for some of them, I couldn't quite tell what the existing code was doing well enough to figure out how to translate them properly into smart pointers and such. (For example, there was a pointer-to-a-pointer -- ** -- and I couldn't figure out if that should actually be a smart pointer to an array, or not.)

The code audit idea is a GREAT idea! Having a more complete picture would help us all immensely to make better decisions, both large and small, about how to proceed from here. IMHO, anyway.

[LifWirser]

Behind the scenes I have been looking both the engine and asset masters. They are far removed from the current 0.5.3 branches respecively, which are the latest created feb 2020. also just compiling engine master does have many "autopointer depreciated compiler warnings but not in 0.5.3 . @stephengtuggy As far I know ATM the 0.5.3 branch is what needs to be worked on , I suspect your "Segfault" was from the older and "obsolete " Engine master judging by the date of you original post .

I know I am throwing alot of information out, Currently I am falling behind on finding issues , I thought I would pick up where I left off understanding the Taose tre into a whole new set of issues in a completely new Vegastrike engine/dataset.

Me personally, am looking forward to running GDB and Valgrind tools on the engine code ( which will show which pointers need first attention)- but that is currently on hold at least until the holes in the base data are correct,

[stephengtuggy]

@LifWirser No worries :sunglasses: Having a little trouble finding the time myself. But very much enjoying the chance to hop in here where I can. :-)

[stephengtuggy]

One note I would have about Valgrind is that, unfortunately, it feels a little too late in the process for me. Like "trying to close the barn door after the cows got out." LOL. Some sort of static analysis tools, providing feedback on the structure of the code itself at each check-in, might be preferable?

All in the spirit of continuous improvement, having patience with ourselves, taking one thing at a time, and catching bugs as early as possible. Hopefully. To the extent that combination is humanly possible. LOL

[evertvorster]

Just now read through the issue.

My personal opinion is that we use the smart pointers, as there are almost no use cases that I can imagine where a developer would use an 8 year old compiler to build a game that won't also be open to updating the compiler to support modern features.

As far as I understand, Vega Strike will be distributed in binary format to the older distrobutions, so the compiler version on those won't make any difference.

[stephengtuggy]

@evertvorster Yes, and what we're discovering, too, is that of the distros that are still supported, most have C++11-compatible compilers by now. With just a few exceptions.

[BenjamenMeyer]

@evertvorster we're all agreed on eventually using smart pointers. The bigger issue is being able to determine which kind of pointer works best in each case. #290 should probably be done first so that we can reduce the places we'd have to make changes; which will also make this easier to roll out.

For example, we presently redefine things like std::vector<sometype> multiple places. We'd have to ensure all those places get updated with the right smart pointer. If we first reduce down to having one typedef version that we use everywhere (see #290 for an example) then making this change would be easier as we'd only have to update a single spot for the storage.