Page 4 of 6

Re: Cracktro :)

Posted: November 1st, 2019, 4:37 pm
by albinopapa
I've been kind of sad/lonely these past few days not being able to discuss this, hopefully this won't be the last of it :).

Re: Cracktro :)

Posted: November 4th, 2019, 9:08 pm
by Byteraver
Haha, no worries, I will come back to the forum every now and then. I'm glad you enjoy it as much as I do. I'm playing with std::unique_ptr, and am in the process of refactoring my code to remove all new/delete combo's, initializer functions (instead of proper constructors), char pointers etc. Big job, we are talking tens of thousands of lines of code. While doing so, I discovered I can do basically everything with std::vector as well. Should I, however? I see no difference in release mode, but in debug mode the program is notably slower. Should I be conservative in replacing classic heap-allocated arrays with std::vector? Or can I go "crazy"?

Second question:
I have a constructor of a class broadly like this ("Note" is a struct containing a fixed amount of bytes, nothing else):

Code: Select all

class Pattern {
public:
    Pattern( unsigned nChannels,unsigned nRows, const std::vector<Note> data ) :
        nChannels_( nChannels ),
        nRows_( nRows )
    {
        size_ = nChannels * nRows;
        assert( size_ > 0 );
        data_ = data;  // no pattern compression for now
    }
private:
    unsigned                nChannels_;
    unsigned                nRows_;
    unsigned                size_;
    std::vector<Note>  data_;
};
Now, how can I make sure the parameter "const std::vector<Note> data" is destroyed after the constructor was called? I want it to be moved to the data_ class member, not copied...

Edit: about your remark on game styles: I wanted to make an RTS game (think StarCraft, Command & Conquer, Dune II), not an RPG game (I never played role playing games, I can see the appeal but it is not my jam I suppose).

Re: Cracktro :)

Posted: November 5th, 2019, 4:15 am
by albinopapa
Damn, I kind of tossed a coin on the acronym, lol. I had forgotten and I like the Age of Empires series myself, especially AoE 2.

Code: Select all

Pattern( unsigned nChannels,unsigned nRows, std::vector<Note> data ) :
        nChannels_( nChannels ),
        nRows_( nRows ),
        size_( nChannels * nRows ),
        data_( std::move( data ) )
    {
        assert( size_ > 0 );
    }
There, all cleaned up and data moved. Anything you want moved, you can't make const.

Here's my opinion on std::vector vs std::unique_ptr. Vector is a container or collection and interacts well with the STL and can be copied provided the objects in the vector can also be copied. Unique_ptr can point to a single object or an array of objects and is just a dumb smart pointer. If you want to iterate over your array using the STL, use std::vector. Another nice feature of vector that unique_ptr doesn't have is the size or element count information. Using std::unique_ptr as an array can be as dangerous as using a C-style array just more flexible. Honestly, std::vector is a win-win really. If you have classes you don't want copied, you can always declare the copy assignment and copy constructors as deleted or if you don't want to accidentally have them copied delete the special functions and create a clone()/copy() function you must call explicitly.

Chili has a video covering std::vector's debug performance and a few steps to speed it up drastically. I'm sure a quick look through his videos will yield the video to which I am referring.

So basically, std::vector has a lot more advantages so go wild. The debug performance "shouldn't" matter all that much, but it is painful sometimes, so check out the video and see if it helps any.

Personally, I do use std::unique_ptr in my wrapper classes or as a local function variable, but other than that I would suggest using unique_ptr for cases where you get back a pointer to an array of data, like from a C or early C++ library or maybe the author just wants their library to be backward compatible. This way you get the safety of the unique_ptr, but you don't have to copy over the elements to an std::vector.

Prefer std::vector if you are using it for a range of things you are going to iterate over either in a loop or STL.
Prefer std::unique_ptr
- for single elements
- local variables that need to die at the end of the function
- wrapper classes where you can store the size and the size isn't going to change during the lifetime of that class ( this is just a personal choice, but it makes sense to me, I have nothing to back this up )
Example: A sprite is a collection of pixels. You most likely aren't going to be adding or removing pixels, so I would use a unique_ptr, but for a series of sprites for an animation I most certainly would choose a vector<Sprite> in that case. Not because I need to add/remove "frames", but because I want the size information and would like to iterate over the entire range sequentially.
- As a way of safely dealing with libraries that return back a raw pointer, either a single element or an array of elements. I'd probably want to create a struct with size and the unique_ptr though for the array cases, or at the very least std::pair<std::unique_ptr<char[]>, size_t>.
Example:

Code: Select all

std::pair<std::unique_ptr<char[]>, size_t> readFileToBuffer( std::string filename )
{
    // Open the file at the end in binary mode
    auto file = std::ifstream( filename, std::ios::ate | std::ios::binary );

    // Verify file is open
    if( !file.is_open() )
        throw std::runtime_error( "File not found, or whatever." );

    // The end position is going to be the size of the file in bytes
    auto const buffer_length = file.tellg();

    // Reset position back to beginning
    file.seekg( 0 );

    // Create a temp buffer, just cleaner this way
    auto temp_buffer = std::make_unique<char[]>( buffer_length );

    // Read in the file contents
    file.read( temp_buffer.get(), buffer_length );

    // Create the std::pair by moving the buffer and length into it and return
    return { std::move( temp_buffer ), buffer_length };
}

// Get the buffer from the function
auto safe_buffer = readFileToBuffer( "binfile.data" );

// With std::pair, you can use C++17's structured bindings to get references to the members of pair.
auto[buffer, size] = safe_buffer;

// You can check buffer for nullptr and size == 0
assert( buffer != nullptr && size != 0 );

// This way you can use the unique_ptr directly and you know it's count/size
int headerSize = 0;
memcpy( &headerSize, &buffer[ 0 ], sizeof( headerSize ) );

// yada, yada

Re: Cracktro :)

Posted: November 5th, 2019, 4:27 am
by albinopapa
I know how you feel about the refactoring. I have a project from about 5-ish years ago that I am pretty much just redoing because I figure it's just as easy to start over as it is to update the code. Well, that's not entirely true. If it were just refactoring the new/delete calls to std::unique_ptr that only took less than an hour I think, it's been awhile since I did that part of it. It's the raw loops and interactions between classes that made it "spaghetti code". Also, most everything was in a single struct and I was just passing the struct around by reference to literally everything else. Was the only way I knew of to allow access to player position so enemies could target you or for the Store to know which weapons you had unlocked or bought already.

Actually, I'm still trying to think of a good way of dealing with it. Some things I can probably use the Observer pattern, but I really don't like type-erasure and having to rely on some message id to tell me what type of data to cast to which I think is a pretty common method. I'll have to look up some other methods.

Anyway, nice chatting as always.

Re: Cracktro :)

Posted: November 6th, 2019, 8:22 pm
by Byteraver
Hey Albinopapa! Thanks for the swift help. I'm in full refactoring frenzy. Memory leaks appeared during the process, then disappeared again (luckily) ;)
I remember Chili putting the sprite in a vector (if I'm not mistaken) and doing some debug-mode performance tuning. Have to find that video back, will do so when I need it. Right now I'm focused on https://github.com/TheRealByteraver/XMPlayer as I got rather far in that project. Still a lot to do, when I'm done I'll tell you so you can have a look if you're interested (no point in doing that right now).

About the refactoring, I have a question choosing variable and class names. Do you ever abbreviate? Or do you keep using stuff like:

Code: Select all

PatternHeader patternHeader;
SampleHeader sampleHeader;
Instrumentheader InstrumentHeader;

(...)
instrumentHeader.samples[sampleNr.length = sourceFile.InstrumentList[InstrumentNr].length;
I changed these long variable names into shorter versions like "instHdr" and "smpHdr" but kept the class names long. I'm not sure if I like the result :) Because now I have to abbreviate all my variable for consistency reasons. Right? I mean, I want to stay below 80 characters per line here ;)

Re: Cracktro :)

Posted: November 7th, 2019, 10:14 am
by albinopapa
Well, here's the only advice I can give you about naming. The names have to mean something. Let's use InstHdr for as an example. the 'hdr' part can be pronounced without all the vowels so I can see that part. The 'Inst' part has to be in constext or else it could means 'INSTance'. Now, I understand an 'instanceHeader' probably doesn't make sense, but instHdr doesn't really say what it is other than maybe a header of some kind. However, I'm not going to nitpick over this AS LONG AS your functions are short enough to be able to refer back to the declaration of the variable if needed. Nothing like having to scroll through 50+ lines to remember what a variable is.

As far as answering your question, of course I abbreviate. I definitely don't write out 'index' all the time I may shorten to just 'i' or 'j' or even 'idx' sometimes. I also shorten 'iterator' to 'it' depending on it's use.

Code: Select all

auto find_it = std::find_if(
    begin( vec ), 
    end( vec ),
    []( auto const & element ){ return element.value() > 0; } 
);
auto rem_it = std::remove_if( 
    begin( vec ), 
    end( vec ), 
    []( auto const& element ){ return element.health() <= 0.f; } 
);
for( auto it = begin( vec ); it != end( vec ); ++it )
{
    *it = 42; 
}
If your functions have to be lengthy, make sure to only define things right before using them so that they are near enough to the place you used them that you don't have to scroll to the top of the function to find the type.

For the most part though, I do try to be descriptive, however I do get lazy. Here's an example from a project I'm working on:

Code: Select all

	void takeDamage( EntityVariant& entity_, float amount_ )noexcept;
So the type is EntityVariant and the variable name is just entity_. I know this is misleading to anyone else that may read this code, and I could have used 'entityVariant_' instead, actually I did have entityVariant_ to begin with, but got tired of writing it out for every function. You might ask, which is it? an entity? or a variant of entity types?

Code: Select all

	void stop( EntityVariant& entity_t )noexcept;
I also seem to be inconsistant with naming when I'm designing the structure of the code, but I do eventually settle on a name that is "self documenting".

Code: Select all

	void chooseMenuState( int selection_ )noexcept;
Here, I do use more descriptive names:

Code: Select all

	void doMenuState( shooter::MainMenu& menu_, shooter::MenuVariant& menuVariant, int selection_ )noexcept
The first variable 'menu_' isn't declared as 'mainMenu_' for instance because there aren't any other menus to deal with inside the function, however, I did want to differentiate the MainMenu param and the MenuVariant param as they both represent a "menu" type, but one is a menu and the other is a variant holding a menu. I've want to write menuVar all the time, but most of the time 'var' is short for VARiable, so I decided against it.

If you do any template programming at some point, you might see others writing something like:

Code: Select all

template<class T> 
constexpr void add( T const& lhs, T const& rhs )noexcept{ return lhs + rhs; }

Code: Select all

	template<typename ContainerValueType>
If you read the thread I made about Range based for loops I posted some code for some iterators and an adapter to return back a std::pair<size_t, ContainerValueType&>. As stated, I could have just as easily wrote: std::pair<size_t, T&> because T is almost universally recognized as the default "go to" template parameter. So why choose ContainerValueType? I wanted to be explicit about what type is returned in the std::pair or what type the iterators are going to need from the user.

One of the reasons I choose to be descriptive with the variable names is because I don't want to write comments. For the most part, I only comment code if it is unclear what the code does and pretty much never have to comment what the variable is suppose to be.

Code: Select all

	template<typename T> void move( T& entity_, float delta_time_ )noexcept;
	template<typename EnemyType> void update( EnemyType& enemy_, float delta_time_ )noexcept
I do use T however when I am not particular about the type to pass in with regard to what the name implies ( entity_ ). As you can see, here I do care about the type being passed in, so I am more explicit. Now, just because you name the template parameters something specific, doesn't mean it is any different than just using T, so I could pass an 'int' and it would accept it, but IF I'm paying attention I see that I need to pass an enemy object to this function.

Code: Select all

	auto cur = *this;
	++( *this );
	return cur;
There are times I choose to really shorten my names if I think they are generally accepted abbreviations like 'cur' meaning 'current'. I could have been more explicit, but the function is short enough to understand that cur is a copy of whatever '*this' is while *this gets incremented, cur remains the same. In other projects, I've used 'auto old = *this;' which is probably more clear.

This is all code from my current project and not just made up on the spot. After having this discussion I decided to be more diligent with the things I suggested for you to do. One of the things that I ended up doing that I'm NOT use to doing is taking the "machinery" out of functions and moving it to supporting functions:

Code: Select all

	template<typename EnemyType> void update( EnemyType& enemy_, float delta_time_ )noexcept
	{
		if( isAtWaypoint( enemy_ ) )
		{
			advanceWaypoint( enemy_ );
			if( pathFinished( enemy_ ) )
			{
				die( enemy_ );
				stop( enemy_ );
			}
			else
			{
				changeDirection( enemy_, Vector2::normalize( enemy_.m_position - activeWaypoint( enemy_ ) ) );
			}
		}

		move( enemy_, delta_time_ );
	}
Hopefully, it's easy to tell what is going on in this function. Compare this to what it use to be:

Code: Select all

	template<typename EnemyType> void update( EnemyType& enemy_, float delta_time_ )noexcept
	{
		if( Vector2::length_sqr( enemy_.m_position - enemy_.waypoints[ enemy_.m_waypoint_index ] ) <
			sqr( Constants::ship_radius ) )
		{
			++enemy_.m_waypoint_index;;
			if( enemy_.m_waypoint_index >= enemy_.waypoints.size )
			{
				entity_.m_health = 0.f;
				entity_.m_velocity = Vec2f{};
			}
			else
			{
				auto const direction = Vector2::normalize( enemy_.m_position - enemy_.waypoints[ enemy_.m_waypoint_index ] )
				entity_.m_velocity = direction_ * entity_.m_max_speed;
			}
		}

		entity_.m_position += entity_.m_velocity * delta_time_;
	}
The former is way more clear and descriptive than the latter version.

Re: Cracktro :)

Posted: November 7th, 2019, 10:17 am
by albinopapa
Note: The latter version has some "bugs" since it was just a copy/paste of the contents of those functions from the former version into the latter version. Glad I used this particular function for the example though, I had some bugs in my actual code that I spotted after pasting the first version.

Re: Cracktro :)

Posted: November 7th, 2019, 10:20 am
by albinopapa
Also, this code would be even cleaner if I wasn't using a procedural design. I have no idea why I chose this method over an OOP approach, just felt like it I guess.

Re: Cracktro :)

Posted: November 7th, 2019, 9:24 pm
by Byteraver
Hi Albinopapa! Thanks again for the great insights. Your example of how you took out "the machinery" from a function surely struck a chord, the appeal is clear.
Question: do you always / mostly write your code / classes / functions in template form? Is that "a thing" now? It seems a bit overly complicated to me. Unless you want to transform your 2D project into a 3D project halfway the developing phase, I'm not sure if I can see the appeal ;)

Re: Cracktro :)

Posted: November 8th, 2019, 1:43 am
by albinopapa
Templates have been a huge part of my coding practice lately. One nice thing about templates is the ability to write common interfaces for multiple types. The update function template for instance allows me to pass in any enemy class that behaves the same, but with different data. Normally you'd use runtime polymorphism, but with templates the function is instantiated with the type built in so it can be more efficient. Also, there's code reuse for objects that behave the same or have similar interfaces. I have 5 enemy types ( unimaginatively named Enemy1, Enemy2, ...Enemy5 ), so instead of writing 5 different functions I get to write one function for all five and no base class.

On the other hand, for types that don't behave the same, I still have to overload the functions though:

Code: Select all

	void update( Boss1& boss_, float delta_time_ )noexcept;
	void update( Boss2& boss_, float delta_time_ )noexcept;
	void update( Projectile& ammo_, float delta_time_ )noexcept;
	void update( EntityVariant& entityVariant_, float delta_time_ )noexcept;
As stated before, the EntityVariant version is a dispatch function to one of the other functions. If you do use std::variant at some point, you are going to want to learn some template programming along with it. Is it complicated? Yes. Overly complicated? Doesn't have to be, but it can be.

Here's an example of something that confuses me.

Code: Select all

	template<typename T> struct is_enemy
	{
		static constexpr bool value = std::disjunction_v<
			std::is_same<T, Enemy1>,
			std::is_same<T, Enemy2>,
			std::is_same<T, Enemy3>,
			std::is_same<T, Enemy4>,
			std::is_same<T, Enemy5>
		>;
	};

	template<typename T> constexpr bool is_enemy_v		= is_enemy<T>::value;

	template<
		typename EnemyType,
		std::enable_if_t<is_enemy_v<EnemyType>, int> = 0
	> void update( EnemyType& enemy_, float delta_time_ )noexcept;
Here's the "legit" version of this function declaration.

I make a type trait the compiler can use to determine if a type is an EnemyType ( struct is_enemy )
I make a convenience variable to keep things sane ( is_enemy_v<EnemyType> instead of is_enemy<EnemyType>::value )
I use std::enable_if_t to prevent this function from being considered for any type that doesn't satisfy is_enemy_v.
Is there a lot of boiler plate code? HELL YES. While it would have been faster to just write 5 functions and just copy/paste the code 5 times, that still means if I add or change something in 1 I'm probably going to want to add or change the same thing in all 5. Not to mention all the other common functions that would have 5 copies. I tried the non-template route...it was a YUGE mess.

Code: Select all

	// Common functions for all entities

	void takeDamage( Hero& entity_, float amount_ )noexcept;
	RectF boundingBox( Hero const& entity_ )noexcept;
	void move( Hero& entity_, float delta_time_ )noexcept;
	void stop( Hero& entity_t )noexcept;
	void changeDirection( Hero& entity_t, Vec2f const& direction_ )noexcept;
	void die( Hero& entity_t )noexcept;

	void takeDamage( Enemy1& entity_, float amount_ )noexcept;
	RectF boundingBox( Enemy1 const& entity_ )noexcept;
	void move( Enemy1& entity_, float delta_time_ )noexcept;
	void stop( Enemy1& entity_t )noexcept;
	void changeDirection( Enemy1& entity_t, Vec2f const& direction_ )noexcept;
	void die( Enemy1& entity_t )noexcept;

	void takeDamage( Enemy2& entity_, float amount_ )noexcept;
	RectF boundingBox( Enemy2 const& entity_ )noexcept;
	void move( Enemy2& entity_, float delta_time_ )noexcept;
	void stop( Enemy2& entity_t )noexcept;
	void changeDirection( Enemy2& entity_t, Vec2f const& direction_ )noexcept;
	void die( Enemy2& entity_t )noexcept;

	void takeDamage( Enemy3& entity_, float amount_ )noexcept;
	RectF boundingBox( Enemy3 const& entity_ )noexcept;
	void move( Enemy3& entity_, float delta_time_ )noexcept;
	void stop( Enemy3& entity_t )noexcept;
	void changeDirection( Enemy3& entity_t, Vec2f const& direction_ )noexcept;
	void die( Enemy3& entity_t )noexcept;

	void takeDamage( Enemy4& entity_, float amount_ )noexcept;
	RectF boundingBox( Enemy4 const& entity_ )noexcept;
	void move( Enemy4& entity_, float delta_time_ )noexcept;
	void stop( Enemy4& entity_t )noexcept;
	void changeDirection( Enemy4& entity_t, Vec2f const& direction_ )noexcept;
	void die( Enemy4& entity_t )noexcept;

	void takeDamage( Enemy5& entity_, float amount_ )noexcept;
	RectF boundingBox( Enemy5 const& entity_ )noexcept;
	void move( Enemy5& entity_, float delta_time_ )noexcept;
	void stop( Enemy5& entity_t )noexcept;
	void changeDirection( Enemy5& entity_t, Vec2f const& direction_ )noexcept;
	void die( Enemy5& entity_t )noexcept;

	void takeDamage( Boss1& entity_, float amount_ )noexcept;
	RectF boundingBox( Boss1 const& entity_ )noexcept;
	void move( Boss1& entity_, float delta_time_ )noexcept;
	void stop( Boss1& entity_t )noexcept;
	void changeDirection( Boss1& entity_t, Vec2f const& direction_ )noexcept;
	void die( Boss1& entity_t )noexcept;

	void takeDamage( Boss2& entity_, float amount_ )noexcept;
	RectF boundingBox( Boss2 const& entity_ )noexcept;
	void move( Boss2& entity_, float delta_time_ )noexcept;
	void stop( Boss2& entity_t )noexcept;
	void changeDirection( Boss2& entity_t, Vec2f const& direction_ )noexcept;
	void die( Boss2& entity_t )noexcept;

	void takeDamage( EntityVariant& entity_, float amount_ )noexcept;
	RectF boundingBox( EntityVariant const& entity_ )noexcept;
	void move( EntityVariant& entity_, float delta_time_ )noexcept;
	void stop( EntityVariant& entity_t )noexcept;
	void changeDirection( EntityVariant& entity_t, Vec2f const& direction_ )noexcept;
	void die( EntityVariant& entity_t )noexcept;

OR

Code: Select all

	template<typename T> void takeDamage( T& entity_, float amount_ )noexcept;
	template<typename T> auto boundingBox( T const& entity_ )noexcept;
	template<typename T> void move( T& entity_, float delta_time_ )noexcept;
	template<typename T> void stop( T& entity_ )noexcept;
	template<typename T> void changeDirection( T& entity_, Vec2f const& direction_ )noexcept;
	template<typename T> void kill( T& entity_ )noexcept;

	void takeDamage( EntityVariant& entity_, float amount_ )noexcept;
	RectF boundingBox( EntityVariant const& entity_ )noexcept;
	void move( EntityVariant& entity_, float delta_time_ )noexcept;
	void stop( EntityVariant& entity_t )noexcept;
	void changeDirection( EntityVariant& entity_t, Vec2f const& direction_ )noexcept;
	void die( EntityVariant& entity_t )noexcept;
I haven't written the enable_if constraints for these functions yet, but regardless templates allow me to pass any type to the function and if the interface of the type I pass matches then I get code reuse which is way awesome.