Page MenuHomeWildfire Games

Use all three color channels when loading heightmaps following rP21113
ClosedPublic

Authored by Silier on Apr 4 2019, 9:04 PM.

Details

Summary

In rP21113, line 68 makes to pick the red channel instead of the colour channel with highest value as intended by comment and code on line 67

(The facts that heightmaps are grayscaled images { every pixel has all 3 colour channel values identical } and that it is working until now could be enough to be sure this is not going to beak current map generation)


(I think if inlined it would not look good and be harder to understand what is going on there)

Test Plan

compile
generate random maps
check there is nothing crazy on them

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7101
Build 11592: Vulcan BuildJenkins
Build 11591: arc lint + arc unit

Event Timeline

Silier created this revision.Apr 4 2019, 9:04 PM
Owners added a subscriber: Restricted Owners Package.Apr 4 2019, 9:04 PM
Vulcan added a subscriber: Vulcan.Apr 4 2019, 9:06 PM

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

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

elexis retitled this revision from Remove wrong assignment introduced in rP2113 to Remove wrong assignment introduced in rP21113.Apr 5 2019, 2:18 AM
elexis edited the summary of this revision. (Show Details)
elexis added a comment.Apr 5 2019, 2:31 AM

Don't forget to add the references in any text field (rP21113), this way the referenced site will also receive a link to this document and readers can click their way through history.

(I think if inlined it would not look good and be harder to understand what is going on there)

Should be fine with newlines:

heightmap[(tileSize - y) * (tileSize + 1) + x] = clamp(
	256 * std::max({mapdata[offset],  mapdata[offset + bytesPP], mapdata[offset + bytesPP * 2]}),
	0,
	65535);

Price question: do we need the cast to u16?

(The facts that heightmaps are grayscaled images { every pixel has all 3 colour channel values identical } and that it is working until now could be enough to be sure this is not going to beak current map generation)

So I guess one can test whether it's actually the case (all greyscale). And to test the patch, one should test random maps that actually come with such a heightmap.
But yes, I don't see this diff failing either.

Silier updated this revision to Diff 7674.Apr 5 2019, 10:43 AM
Silier edited the summary of this revision. (Show Details)
Silier added a subscriber: vladislavbelov.

Pictures are white-black or white-gray-black.
Maps looks good I think.

clamp has int as parameters (32 bits), std::max returns only 8 bits so might not bee needed. But I am not sure about that.
@vladislavbelov ?

What does the C++ specs say what happens?

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

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

Stan added a subscriber: Stan.Apr 5 2019, 11:12 AM

In CPP casting to a smaller type will remove the leading byte.
https://stackoverflow.com/questions/6752567/casting-a-large-number-type-to-a-smaller-type

source/graphics/MapIO.cpp
94

One could also keep that to an explicitly named variable to increase readability.

vladislavbelov added a comment.EditedApr 5 2019, 11:37 AM
In D1816#74161, @Angen wrote:

clamp has int as parameters (32 bits), std::max returns only 8 bits so might not bee needed. But I am not sure about that.

Clamp is a template function, so a compiler deduce a type from its arguments.

In D1816#74166, @elexis wrote:

What does the C++ specs say what happens?

The cast isn't required here.

Also the expression 256 * std::max({mapdata[offset], mapdata[offset + bytesPP], mapdata[offset + bytesPP * 2]}) can't be less than 0 and greater than 65535. Because the mapdata is u8. And the clamp is useless here until we have a bigger type for mapdata. So the question is what we can do to prevent overflows/underflows in future when someone will change types.

I'd prefer to remove Clamp, because if we'll increase the mapdata type a compiler will highly likely report about it.

source/graphics/MapIO.cpp
93

Clamp.

elexis added a comment.Apr 5 2019, 4:25 PM

(I meant we can find the answer in the specs if we are not sure)

Silier updated this revision to Diff 7719.Apr 10 2019, 9:33 PM

remove not needed clamp
mapdata is u8 (0 - 255), maxed value we can get here is 65280 what is in range of u16 (0 - 65535)

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

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

elexis retitled this revision from Remove wrong assignment introduced in rP21113 to Use all three color channels when loading heightmaps following rP21113.Apr 25 2019, 2:01 PM
elexis accepted this revision.Sep 12 2019, 9:19 PM

As mentioned in https://code.wildfiregames.com/rP21113#inline-2823, I can only imagine that this was a debug line that I left over that wasn't noticed because the code produces the same result for our heightmap data.

On clamp from source/maths/MathUtil.h, that is type agnostic indeed:

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

eh uppercase Clamp from source/lib/lib.h too:

template<typename T>
T Clamp(T val, T min, T max)
{
	ASSERT(min <= max);
	return std::max(min, std::min(val, max));
}

Confirming what Vladislav says, 256 * u8 is always greater 0 and always smaller than 255 * 256, i.e. always fitting into an u16 (type of heightmap), so clamp unneeded!

The useless clamp comes from rP12308 and was recklessly copied by me. It seems at the time it also had a useless u16 conversion.

The result type of 256 * u8 is determined by https://en.cppreference.com/w/c/language/conversion#Usual_arithmetic_conversions

both operands undergo integer promotions

The type of 256 is int or greater
https://en.cppreference.com/w/cpp/language/integer_literal
and int is 16 bit or greater
http://www.cplusplus.com/doc/tutorial/variables/

That should be the theory matching the experimental success of the patch.
The previous code from myconid looks like it had done the u16 conversion to ensure the type before.

....

See IRC discussion about the type, we agreed to use static_cast<u16>(256): http://irclogs.wildfiregames.com/2019-09/2019-09-12-QuakeNet-%230ad-dev.log

Thanks for discovering it and uploading the patch Angen!
Sorry for the delay!!

source/graphics/MapIO.cpp
93

+\n (so that each item is on a separate line symmetrically)

This revision is now accepted and ready to land.Sep 12 2019, 9:19 PM
Silier updated this revision to Diff 9725.Sep 12 2019, 9:19 PM

static cast to ensure u16 result

Otherwise, the signedness is different and the signed operand's rank is greater than unsigned operand's rank. In this case, if the signed type can represent all values of the unsigned type, then the operand with the unsigned type is implicitly converted to the type of the signed operand.
(https://en.cppreference.com/w/c/language/conversion#Usual_arithmetic_conversions)

[21:06] <elexis> int literal = i16 or greater
(http://irclogs.wildfiregames.com/2019-09/2019-09-12-QuakeNet-%230ad-dev.log)

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

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

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

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