Page MenuHomeWildfire Games

Use all three color channels when loading heightmaps following rP21113
Needs ReviewPublic

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

Details

Reviewers
elexis
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 7155
Build 11667: Vulcan BuildJenkins
Build 11666: arc lint + arc unit

Event Timeline

Angen 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.

Angen updated this revision to Diff 7674.Apr 5 2019, 10:43 AM
Angen edited the summary of this revision. (Show Details)
Angen 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
89

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
88–90

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)

Angen 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