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.
Details
- Reviewers
wraitii Itms - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22051: Split CColor from Shapes.
- Apply the patch
- Compile the game
- 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 Build Jenkins
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/543/display/redirect
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. |
source/graphics/Color.cpp | ||
---|---|---|
103 | Ah nvm. |
source/graphics/Color.h | ||
---|---|---|
55 | Isn't r a single value ? |
source/graphics/Color.h | ||
---|---|---|
55 | No, it's an assumption, that r, g, b, a values are sequentially stored in the memory. |
source/graphics/Color.cpp | ||
---|---|---|
100 | So why int ? It's not like it can be negative nor exceed the size of u8 ? |
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. |
source/graphics/Color.cpp | ||
---|---|---|
100 | In this case i16 might work. Is HDR using rgba ? |
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. |
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?) |
Yeah, compiling with/without PCH and running the game and tests would be enough here.
source/graphics/Color.cpp | ||
---|---|---|
143 | It presents here :) |
Can confirm it builds and works on windows, with or without pch. To compile without pch you'll need D1361
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. |
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 :) |