Page MenuHomeWildfire Games

Split CColor from Shapes
ClosedPublic

Authored by vladislavbelov on May 22 2018, 2:20 AM.

Details

Reviewers
wraitii
Itms
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22051: Split CColor from Shapes.
Summary

Currently CColor is in ps/Shapes.h, that seems incorrect. Also we use Color more frequently than shapes, so we don't need to include them all.

Test Plan
  1. Apply the patch
  2. Compile the game
  3. Run tests

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6120
Build 10188: Vulcan BuildJenkins

Event Timeline

vladislavbelov created this revision.May 22 2018, 2:20 AM
Vulcan added a subscriber: Vulcan.May 22 2018, 2:40 AM

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

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

Stan added a subscriber: Stan.Jun 3 2018, 11:36 PM

Some comments. Other than that patch looks good.

source/graphics/Color.cpp
100

Since when params have caps ? Int seems like it could be replaced by u8

103

Same here.

103

Ugh can't we just create a color here ?

Actually since it's not used what's the point ?

109

Range loop ?

126

Can't that be used to early return ? Same for below.

source/graphics/Color.h
46

Maybe a good candidate for STD::limits if applicable.

51

Can't we write

return *this != color ?

Hence removing that block. ?

55

How is that an array ?

60

Could use STD limits but seems overkill. I don't get the 0 though. If it's supposed to be a float why no static_casts and why no f suffix.

Stan added inline comments.Jun 3 2018, 11:49 PM
source/graphics/Color.cpp
103

Ah nvm.

vladislavbelov added inline comments.Jun 4 2018, 12:11 AM
source/graphics/Color.cpp
100

Never, just copy & pasted. Thanks.

source/graphics/Color.h
51

Nope, *this != color is equal to this->operator!=(color), but you try to define it. So it'd be a recursion.

55

What do you mean?

Stan added inline comments.Jun 4 2018, 12:48 AM
source/graphics/Color.h
55

Isn't r a single value ?

vladislavbelov added inline comments.Jun 4 2018, 1:30 AM
source/graphics/Color.h
55

No, it's an assumption, that r, g, b, a values are sequentially stored in the memory.

vladislavbelov marked 2 inline comments as done.

Fixes notes.

vladislavbelov added inline comments.Jun 4 2018, 6:31 PM
source/graphics/Color.cpp
109

You need a counter anyway.

126

It's already the early return.

source/graphics/Color.h
46

STL doesn't have limits for RGB.

Stan added inline comments.Jun 4 2018, 6:39 PM
source/graphics/Color.cpp
100

So why int ? It's not like it can be negative nor exceed the size of u8 ?

vladislavbelov added inline comments.Jun 4 2018, 6:47 PM
source/graphics/Color.cpp
100

Because negative values may be used as wrong values, i.e. if you'll take a look at the CColor, you'd see, that it's initialised by -1s. Not i8 or u8, because we can have an HDR color in the future, and 255 is not the limit.

Stan added inline comments.Jun 4 2018, 6:49 PM
source/graphics/Color.cpp
100

In this case i16 might work. Is HDR using rgba ?

Stan added inline comments.Jun 4 2018, 6:55 PM
source/graphics/Color.cpp
121

Could use the constant above minus one

126

Shouldn't it be an else if ?

143

I thought checking for float equality was bad ?

source/graphics/Color.h
55

How is that a good assumption ? Can't we just return an array ?

vladislavbelov added inline comments.Jun 4 2018, 8:32 PM
source/graphics/Color.cpp
100

Why i16? It may be i32. So I leave this as is, until we consider what do we exactly need.

121

Not sure, because we know RGB = 3 components (even HSL and HSV = 3 components).

126

No, it shouldn't. Because we need check conditions anyway, even one of them is absent.

143

Yes, you're right, it's not good to compare floats without an epsilon. But I leave as is, because it may break something and I will need to repair it, and the diff will be much bigger. This diff is mostly for split.

elexis added a subscriber: elexis.Nov 21 2018, 9:23 AM

I agree with the patch, it's a good cleanup. A Color entirely is not a Shape and it will leave behind cleaner code if we don't include the shapes header if we only need the color.
(I suppose one could lose a lot of time reinvestigating the completeness of includes, better don't even start).
Otherwise I suppose one could test it by compilign without precompiled headers, running every function that uses colors and then it could be committed if it still doesn't fall apart and everything is confirmed to be the same code flow as before.

source/graphics/Color.cpp
143

(Did I hear someone speaking of a test_Color file?)

vladislavbelov marked an inline comment as done.Nov 21 2018, 9:32 PM
In D1515#66454, @elexis wrote:

I agree with the patch, it's a good cleanup. A Color entirely is not a Shape and it will leave behind cleaner code if we don't include the shapes header if we only need the color.
(I suppose one could lose a lot of time reinvestigating the completeness of includes, better don't even start).
Otherwise I suppose one could test it by compilign without precompiled headers, running every function that uses colors and then it could be committed if it still doesn't fall apart and everything is confirmed to be the same code flow as before.

Yeah, compiling with/without PCH and running the game and tests would be enough here.

source/graphics/Color.cpp
143

It presents here :)

Stan added a comment.Dec 28 2018, 3:08 PM

Can confirm it builds and works on windows, with or without pch. To compile without pch you'll need D1361

wraitii added inline comments.Jan 7 2019, 8:47 PM
source/graphics/Color.cpp
48

Some very basic profiling on my computer shows that this function is _way_ slower than the corresponding optimised code from the above fallback. I'd suggest deleting this. Maybe for another revision though.

(I've compiled https://pastebin.com/JBxJzjgM with `-std=c++11 -O3 -msse3 -march=native``` and it's about 280ms for the fallback and 700 for the see version).

121

why is there even a constant TBH.

143

In this case it is fine because we only want to return equality if the floats are the same. It's comparing floats to integer (or vice-versa) that's super dangerous, but float-float is fine.

Stan added inline comments.Jan 7 2019, 9:54 PM
source/graphics/Color.cpp
143

Interesting in C# I can't rely on stuff like that. If I do so and use a bunch of vectors it get messed up pretty quick :)

vladislavbelov marked 2 inline comments as done.Jan 13 2019, 3:17 PM
vladislavbelov added inline comments.
source/graphics/Color.cpp
48

Ok, I'll measure the implementation after this patch.

121

Because it was in the original implementation, it's only moved.

wraitii accepted this revision.Jan 13 2019, 3:18 PM
This revision is now accepted and ready to land.Jan 13 2019, 3:18 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jan 13 2019, 4:38 PM

I made a performance test for comparison: P149.