Page MenuHomeWildfire Games

Removes duplication of Clamp function
ClosedPublic

Authored by vladislavbelov on Jan 24 2019, 9:52 PM.

Details

Reviewers
wraitii
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23002: Removes duplication of Clamp function.
Summary

There was 2 similar implementation of the Clamp function. I left the MathUtil implementation, because I think the function is about math abstraction, not about low level operations.

Test Plan
  1. Apply the patch.
  2. Compile the game without PCH.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
vladislavbelov marked an inline comment as done.Jan 24 2019, 11:38 PM

Fixes another Stan notes.

source/graphics/Color.cpp
37 ↗(On Diff #7390)

SColor4ub - yes.

Stan added inline comments.Jan 24 2019, 11:44 PM
source/graphics/HeightMipmap.cpp
109 ↗(On Diff #7390)

Still missing :P

125 ↗(On Diff #7390)

Here as well :)

source/graphics/ParticleEmitterType.cpp
591 ↗(On Diff #7391)

static_cast, spaces

source/graphics/Terrain.cpp
413 ↗(On Diff #7390)

spaces

346 ↗(On Diff #7389)

Not done :)

351 ↗(On Diff #7389)

Here too :D

Fixes other another Stan notes.

Stan added a comment.EditedJan 24 2019, 11:59 PM

Builds in debug and release configurations on windows without pch.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1001/

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1002/

elexis added a subscriber: elexis.Jan 25 2019, 2:53 PM
elexis added inline comments.
source/maths/MathUtil.h
41 ↗(On Diff #7392)

I heard we avoid else before return.
(If there is code, people think it has a use, but these elses don't. Secondly it costs a bit of time to read characters)

source/simulation2/components/CCmpPosition.cpp
789 ↗(On Diff #7392)
source/simulation2/helpers/Render.cpp
91 ↗(On Diff #7392)

(I read people asked about static casts?)

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
53 ↗(On Diff #7389)

numeric_limit for that 2^16?

smiley added a subscriber: smiley.Jan 25 2019, 3:04 PM

(One can argue that something worse than an inconsistent code base is inconsistency within a single file. But then again, such arguments are probably what led to this situation in the first place)

Stan added inline comments.Jan 25 2019, 8:03 PM
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
53 ↗(On Diff #7389)

Yeah would be nice. Same below.

vladislavbelov marked 2 inline comments as done.

Fixes elexis and another other another Stan notes.

source/simulation2/components/CCmpPosition.cpp
789 ↗(On Diff #7392)

Hmm...

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
53 ↗(On Diff #7389)

It'd be good if you test that the std::numeric_limits<u16>::max() is correct for you.

Stan added inline comments.Jan 25 2019, 9:07 PM
source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
53 ↗(On Diff #7389)

in source\lib\types.h

typedef uint8_t u8;
typedef uint16_t u16;
typedef uint32_t u32;
typedef uint64_t u64;

MSVC

c:\Users\dolci\Desktop>cl.exe fine.cpp /EHsc

c:\Users\dolci\Desktop>fine.exe
std::numeric_limits<u16>::max(): 65535

MINGW

c:\Users\dolci\Desktop>mingw32-g++ fine.cpp

c:\Users\dolci\Desktop>a.exe
std::numeric_limits<u16>::max(): 65535

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1003/

(Re: ssize_t to size_t
I thought that this was discouraged since it can introduce subtle bugs.
Google's C++ style guide also mentions something like this)

In D1763#71069, @smiley wrote:

(Re: ssize_t to size_t
I thought that this was discouraged since it can introduce subtle bugs.
Google's C++ style guide also mentions something like this)

Yeah, we discusses about it yesterday with Stan. The problem is that ssize_t is used as size_t in some places.

smiley added a comment.EditedJan 25 2019, 9:45 PM
In D1763#71069, @smiley wrote:

(Re: ssize_t to size_t
I thought that this was discouraged since it can introduce subtle bugs.
Google's C++ style guide also mentions something like this)

Yeah, we discusses about it yesterday with Stan. The problem is that ssize_t is used as size_t in some places.

Just pointing it out. You know way more about C++ than me to make the right call :-)

Stan added a subscriber: wraitii.Feb 25 2019, 10:14 AM

Here are some comments while you wait for a good review from @wraitii or someone else.
Maybe you could mark my comments as done so you don't lose track, because you missed some :)

source/graphics/Camera.cpp
305 ↗(On Diff #7390)

Missing spaces between operators.

228 ↗(On Diff #7395)

Missing spaces between operators.

source/graphics/Color.cpp
37 ↗(On Diff #7390)

Could have used std::limits but I guess that's too verbose.

source/graphics/HeightMipmap.cpp
107 ↗(On Diff #7395)

0.0f

source/graphics/Terrain.cpp
455 ↗(On Diff #7395)

Missed a static_cast.

460 ↗(On Diff #7389)

Missing static cast.

source/renderer/WaterManager.cpp
1086 ↗(On Diff #7389)

Spaces between operators.

source/simulation2/components/CCmpTerritoryManager.cpp
365 ↗(On Diff #7395)

Pretty sure this isn't returning a U16

source/simulation2/system/TurnManager.cpp
217 ↗(On Diff #7395)

missing f / static_casts

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
53 ↗(On Diff #7389)

Why not clamp<u16> ?

69 ↗(On Diff #7389)

Why not clamp<u16> ?

Stan requested changes to this revision.Mar 6 2019, 3:11 PM
This revision now requires changes to proceed.Mar 6 2019, 3:11 PM
vladislavbelov marked 13 inline comments as done.

Fixes Stan's notes.

source/graphics/Color.cpp
37 ↗(On Diff #7390)

It requires more changes, as adding a component type as own type.

source/simulation2/components/CCmpTerritoryManager.cpp
365 ↗(On Diff #7395)

It'd be better to fix it separately. Too big patch already.

source/simulation2/system/TurnManager.cpp
217 ↗(On Diff #7395)

No need to use static_cast here, all other elements are float.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
53 ↗(On Diff #7389)

Because you clamping a number more than maximum possible value of u16. If we don't use a bigger type (like int) here, we get an integer overflow.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/MiscHandlers.cpp
|  33| #include·"ps/Util.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ElevationHandlers.cpp
|  33| #include·"../Brushes.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1135/display/redirect

Stan added inline comments.Apr 3 2019, 11:42 AM
source/graphics/Camera.cpp
305 ↗(On Diff #7619)

Is the compiler smart enough to optimize (mapSize - 1) * TERRAIN_TILE_SIZE) ? as it's also used below ? could be inlined in a variable. Here it's a multiplication while others are mostly substractions

source/graphics/HeightMipmap.cpp
66 ↗(On Diff #7619)

Same concern as above.

source/graphics/ObjectBase.cpp
176 ↗(On Diff #7619)

Switch case with fallthrough ?

source/renderer/TerrainRenderer.cpp
287 ↗(On Diff #7619)

Space between operators :P

source/simulation2/helpers/Render.cpp
91 ↗(On Diff #7392)

Why does this one specify Clamp<T> ?

vladislavbelov added inline comments.Apr 3 2019, 1:22 PM
source/graphics/Camera.cpp
305 ↗(On Diff #7619)

Yeah, most compilers calculate this expression once.

source/graphics/ObjectBase.cpp
176 ↗(On Diff #7619)

Nope, switch accepts only constant expressions.

source/simulation2/helpers/Render.cpp
91 ↗(On Diff #7392)

C++ doesn't have number postfix for std::size_t. So I used explicit type to not do explicit static_cast of the constants.

Angen awarded a token.Apr 10 2019, 9:36 PM
Itms added a subscriber: Itms.Apr 10 2019, 9:38 PM
Itms added inline comments.
source/maths/MathUtil.h
26 ↗(On Diff #7619)

why did you remove const here?

40 ↗(On Diff #7619)

+1 to elexis' remark.

Feels a bit like bike shedding at this point. I suppose this is good to go once the 'else's are fixed.

source/maths/MathUtil.h
26 ↗(On Diff #7619)

The syntax looks like pass-by-copy so it may be redundant in most cases, but I suppose we could use pointers as T so I think it'd be worth keeping this const for the sake of clarity.

32 ↗(On Diff #7619)

Likewise here.

40 ↗(On Diff #7619)

Adding +1 as well because why not.

Stan added inline comments.Apr 17 2019, 6:49 PM
source/maths/MathUtil.h
40 ↗(On Diff #7619)

It's been fixed already ^^

Stan resigned from this revision.May 10 2019, 3:10 PM

Not much I can review anymore.

wraitii accepted this revision.May 25 2019, 11:28 AM

fix the const that were removed / sort out what you like for else and commit this.

This revision is now accepted and ready to land.May 25 2019, 11:28 AM

I will commit it in separate commits, grouped by folders.

Updates after last commits.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/747/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/235/display/redirect

vladislavbelov added inline comments.Sep 19 2019, 8:52 PM
source/maths/MathUtil.h
32 ↗(On Diff #7619)

I tried to make it more optimized, but it looks more complicated, than someone may expect:

#include <type_traits>

namespace internal
{

template <typename T>
struct MathArgType
{
	// We check a size of the type to be sure that it might fit in machine
	// register to pass it by value (currently not all compilers inline
	// functions with template arguments optimally).
	using type = typename std::conditional<
		std::is_arithmetic<T>::value && sizeof(T) <= sizeof(void*),
		T,
		const T&>::type;
};

template <typename T, typename Arg>
inline T Clamp(Arg value, Arg min, Arg max)
{
	if (value <= min)
		return min;
	else if (value >= max)
		return max;
	return value;
}

} // namespace internal

template <typename T>
inline T Clamp(T&& value, T&& min, T&& max)
{
    return internal::Clamp<typename std::decay<T>::type, typename internal::MathArgType<T>::type>(value, min, max);
}
This revision was landed with ongoing or failed builds.Sep 26 2019, 11:14 PM
This revision was automatically updated to reflect the committed changes.