C++ Newbie - Please judge my C++ code.

Development Programming & Scripting C++
1

I have a math macro in blueprints:

b7e3fff64d.png
b7e3fff64d.png1896×1097 233 KB

I converted it to C++, and now it looks like this:


void UCustomBlueprintNodes::SnapVectorToGrid(FVector Vector, float GridSize, FVector& SnappedVector)
{
	float SnappedX;
	float SnappedY;
	float SnappedZ;

	if (FMath::Fmod(Vector.X, GridSize) < GridSize / 2)
	{
		SnappedX = (float)((int)(Vector.X / GridSize))*GridSize;
	}

	else
	{
		SnappedX = ((float)((int)(Vector.X / GridSize))*GridSize) + GridSize;
	}

	if (FMath::Fmod(Vector.Y, GridSize) < GridSize / 2)
	{
		SnappedY = (float)((int)(Vector.Y / GridSize))*GridSize;
	}
	else
	{
		SnappedY = ((float)((int)(Vector.Y / GridSize))*GridSize) + GridSize;
	}

	if (FMath::Fmod(Vector.Z, GridSize) < GridSize / 2)
	{
		SnappedZ = (float)((int)(Vector.Z / GridSize))*GridSize;
	}
	else
	{
		SnappedZ = ((float)((int)(Vector.Z / GridSize))*GridSize) + GridSize;
	}


	SnappedVector = FVector(SnappedX, SnappedY, SnappedZ);
}

I am wanting you all to look at my code, and tell me a few things. Am I using order of operations correctly? Is my code organized? What could I do to improve this code?

One thing before you judge that I am already aware of, I know that rather than using if and else statements, I could have just added


+ GridSize*(!FMath::Fmod(Vector.X, GridSize) > GridSize / 2)

to the end of math equation when setting the snappedX/Y/Z values, and only needed 1 line for each vector value, but that is harder to understand in my opinion.

2

Without looking at too much what your code is doing, one area you may want to consider (unless I read it wrong):



void UCustomBlueprintNodes::SnapVectorToGrid(FVector Vector, float GridSize, FVector& SnappedVector)
{
	float SnappedX = (float)((int)(Vector.X / GridSize))*GridSize;
	float SnappedY = (float)((int)(Vector.Y / GridSize))*GridSize;
	float SnappedZ = (float)((int)(Vector.Z / GridSize))*GridSize;

	if (FMath::Fmod(Vector.X, GridSize) >= GridSize / 2)
		SnappedX += GridSize;


	if (FMath::Fmod(Vector.Y, GridSize) >= GridSize / 2)
		SnappedY += GridSize;

	if (FMath::Fmod(Vector.Z, GridSize) >= GridSize / 2)
		SnappedZ += GridSize;

	SnappedVector = FVector(SnappedX, SnappedY, SnappedZ);
}

Also I like to make sure that GridSize is not zero, and would precede the code with a check for zero and assign it one if so.

3

That’s a very interesting, and very clean approach :smiley: Thanks for the advice!

4

Instead of doing float to int to float again… I’d use FMath::Abs, just to make reading little less confusing.

5

Err… how about something like this instead (requires include )?



FVector snap(FVector arg, float gridSize){
	auto f = &](float val){
		return gridSize ? (float)round(val/gridSize)*gridSize: val;		
	};
	return FVector{f(arg.X), f(arg.Y), f(arg.Z)};
}

or:



template<class Func> FVector processVec(FVector arg, Func f){
	return FVector{f(arg.X), f(arg.Y), f(arg.Z)};
}

FVector snap(FVector arg, float gridSize){
	return processVec(arg, &](float val){
		return gridSize ? (float)round(val/gridSize)*gridSize: val;		
	});
}

Test program:



struct FVector{
	float X;
	float Y;
	float Z;
};

#include <cmath>
#include <iostream>

//start of important part

template<class Func> FVector processVec(FVector arg, Func f){
	return FVector{f(arg.X), f(arg.Y), f(arg.Z)};
}

FVector snap(FVector arg, float gridSize){
	return processVec(arg, &](float val){
		return gridSize ? (float)round(val/gridSize)*gridSize: val;		
	});
}

//end of important part

std::ostream& operator<<(std::ostream &os, const FVector& arg){
	os << arg.X << " " << arg.Y << " " << arg.Z << " ";
}


int main(){
	std::cout << snap(FVector{0.1f, 0.1f, 0.1f}, 2.0f) << std::endl;
	std::cout << snap(FVector{1.1f, 0.9f, 3.1f}, 2.0f) << std::endl;
}
6

Why would Abs() be used? The data type conversion is to remove the decimals.

7

I meant Floor, typed abs but in fact didn’t even notice I wrote that… Guess is time to sleep :wink:

8

That does not work properly with negatives (for what I need anyways), and it would be simpler to do the data type conversion than use Ceil or Floor based on it’s sign.

Thanks for your suggestions everyone, I ended up simplifying it quite a bit, and even learned a few things in the process (Ternary Conditional Operators), here is the new code:


void UCustomBlueprintNodes::SnapVectorToGrid(FVector Vector, float GridSize, FVector& SnappedVector)
{
	float SnappedX;
	float SnappedY;
	float SnappedZ;
	FVector AbsVector = FVector(FMath::Abs(Vector.X), FMath::Abs(Vector.Y), FMath::Abs(Vector.Z));
	SnappedX = (float)((int)(Vector.X / GridSize))*GridSize;
	SnappedY = (float)((int)(Vector.Y / GridSize))*GridSize;
	SnappedZ = (float)((int)(Vector.Z / GridSize))*GridSize;

	if (FMath::Fmod(AbsVector.X, GridSize) >= GridSize / 2)
	{
		SnappedX += GridSize*((Vector.X < 0) ? (-1) : (1));
	}

	if (FMath::Fmod(AbsVector.Y, GridSize) >= GridSize / 2)
	{
		SnappedY += GridSize*((Vector.Y < 0) ? (-1) : (1));
	}

	if (FMath::Fmod(AbsVector.Z, GridSize) >= GridSize / 2)
	{
		SnappedZ += GridSize*((Vector.Z < 0) ? (-1) : (1));
	}

	SnappedVector = FVector(SnappedX, SnappedY, SnappedZ);
}
9

I advise to review my previously posted code snippet.



FVector snap(FVector arg, float gridSize){
	auto f = &](float val){
		return gridSize ? (float)round(val/gridSize)*gridSize: val;		
	};
	return FVector{f(arg.X), f(arg.Y), f(arg.Z)};
}

First, code duplication should be eliminated in almost all cases. You have three if/else branches doing the same thing, but on different fields of the structure. This stuff belong to another function or lambda. Duplication results in bugs and increases maintenance costs.

Also, cmath library has round() function, which rounds float/double value to the nearest integer.
Your code does the same thing as this builtin function, so you’re reinventing the wheel. That is a bad practice.

Using round(), you can replace convoluted if/else with



snappedValue = (float)round(val/gridSize)*gridSize;
10

NegInfinity, have you looked at the assembly output of your suggestion:



FVector snap(FVector arg, float gridSize){
	auto f = &](float val){
		return gridSize ? (float)round(val/gridSize)*gridSize: val;		
	};
	return FVector{f(arg.X), f(arg.Y), f(arg.Z)};
}

I thought the same thing too with respect to having a single function being called to calculate the term. However, normally there is the cost of the push/pop/etc. associated with it. I don’t know how auto influences the assembly output. Have you looked at it?

In general, especially with applications that run in resource rich environments or with functions that aren’t being used too frequently, modularizing as much as possible is good programming practice. For me though, and for this specific problem, I wouldn’t incur the cost of the extra function calls. From a knee-jerk evaluation most of the statements can be executed in a linear fashion with the FPU. If the auto and use of it has no cost associated with the stack and execution management then I would use your approach.

If I had time I’d analyze and benchmark the two but I’m busy with other things at the moment.

11

It’s advised to use UE4 equivalents though. FMath::Round for example or FMath::GridSnap. That stuff is optimized for the engine and also provides the necessary platform independence (see FPlatformMath).

12

That is a bad idea when it comes to C++ code.

By default you don’t know what compiler will do with your code. There are absolutely no warranties. Different platform and different compilers will produce different code. You don’t know if it will or will not inline some method, if it will unroll loops, if it will turn reference into pointers or treat them as aliases to variables, and so on. C++ standard does not provide any guarantees there.

Thinking about low-level performance optimizations while writing stuff is a big red flag which indicates premature optimization. Code is written for other humans to read it and the most important thing is readability and reduction of possibility of having bugs happen in the future (avoiding code duplication is one of those techniques). “Profile before optimizing”. Failure to follow this rule will result in a lot of wasted time (say, you’ll make a function run 4x times faster, except it takes 0.1 ns to complete and is called 5 times total during program’s lifetime). There’s no point in worrying about optimizing this function unless it is being called 20 million times per second - 233 mhz cpu could easily do that back in the 1990s…2000s. Also, algorithmic optimization often will produce significantly larger impact than low-level fine-tuning.

If you’re conecerned about low-level performance, I’d like to point out that conversion from float to int can be quite slow. Compiler will generate _ftol instructions for that, and if you have few billions of calls to those, then _ftol will turn into bottleneck.

However, you shouldn’t be concerned about performance unless you have profiling results on your hand.

13

The stuff in standard library is ALSO optimized for your compiler and your target operating system.

It makes sense to use abstraction layer for things that standard library doesn’t provide. For example, network functionality. Floating points don’t really need additional abstraction layer.

Also, Round() is marked as deprecated and it appears to redirect the call to RoundToInt, which converts floating point number to integer.

14

Game studios do not use STL, at least I think most of them moved away from it.
I’ve seen a while ago a GDC talk about “Common C++ Performance Mistakes in Games” and they pretty much bash STL all the way…
For example, EA has their own ‘EASTL’, Epic Games have theirs, and so on.

15

NegInfinity, are you kidding. That’s why we have a lot of bad code as it. For the most part I’m not disagreeing with your other evaluations but what you’re espousing doesn’t negate what I’ve done over the years. Certainly you may be shielded from the different architectures but that doesn’t apply to everyone and every application. You apparently missed my point.

16

Bruno, that’s funny to hear. Sorry I missed it.

STL is slow, and if you don’t know how to use it correctly it gets even worse. I’ve seen it firsthand numerous times. The fact that “game studios” felt it was good to use in the first place is a clear indication that people there do err in judgement, so much so that a talk was given at GDC.

I got to get off this thread. . .I have work to do.

17

STL stands for Standard Template Library. There’s no STL, only standard library currently, mathematical functions are not templated.

I’m not sure what your point was supposed to be, so you might want to state it clearly. For me “bad code” is that is harder to read and has premature optimization applied. Speaking of “doesn’t negate”… technologies aren’t set in stone and keep changing over time, so if something was a good idea in the past, it doesn’t mean it is still a good idea at the present time.

and if we’re talking about standard containers, those are usually slow in the debug build, because, for example, msvc applies a lot of sanity checks and warnings in debug build only. Now, if you want precise memory management and, say, no dynamic memory allocations, that’s different story.

18

I have to ask, what is a premature optimization? Reducing the number of statements is an optimization. Keeping functionality centralized is an optimization. I guess in your mind then these are premature too.

19

So use RoundToFloat (which admittedly is just a cast RoundToInt). Are you going to ignore most of the stuff in FMath and use your own thing instead or are you going to heed your own advice and keep code clean and readable for other devs doing it the UE4 way?

20

I’m no expert at C++ but here’s my readability-over-minor-performance-boost take on it.



FVector UCustomBlueprintNodes::SnapVectorToGrid(FVector Vector, float GridSize)
{
    return FVector(
        ComputeSnappedValue(Vector.X, GridSize)
        , ComputeSnappedValue(Vector.Y, GridSize)
        , ComputeSnappedValue(Vector.Z, GridSize)
        );
}

(Note: I know the weird comma-placing, that’s just my style. Use yours.)

Also, I changed the pass-by-reference to a returned value (if that’s possible with blueprints; not yet checked) since you’re not modifying original values of the referenced variable but the pass-by-value FVector Vector.

As mentioned above, the separate if-statement is for readability which I agree specially when working with teams and improving maintainability.



float ComputeSnappedValue(float Value, float GridSize)
{
    float SnappedValue = int(Value / GridSize) * GridSize;
    if (FMath::Fmod(Value, GridSize) < (GridSize / 2))
    {
        SnappedValue += GridSize;
    }

    return SnappedValue;
}

Just a tip on if-statement, always use brackets even on a single statement. That will protect your code and newbie developers that will modify your code.

Just some few tips:

  • Code reuseability: if you see code duplications (i.e., 3 similar if-stmt), try to branch it as a new function and reuse it. Maintainability-wise, this would save you time in the future.
  • Don’t use pass-by-ref if you’re going to create a new instance of object inside the function anyway (unless its the only way). Use return value instead.