A class that represents colors in Anari and converts them in different formats
Details
Details
- Reviewers
jcmcdonald • gacmix • imirzali - Maniphest Tasks
- T1247: ARGB Colors
- Commits
- rA08ff791afcf4: Color Class
- Required Signatures
Restricted Legalpad Document
run color test suite (tbd)
Diff Detail
Diff Detail
- Repository
- rA Anari
- Branch
- colors
- Lint
Lint Warnings
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| anari-source/include/anari/colors.hpp | ||
|---|---|---|
| 108 | Adding a copy assignment operator would be a good idea. Color& operator=(const Color& rhs) { // definition here } | |
| 153 | An explicit constructor is one that doesn't attempt to implicitly cast between types. Typically, we make single-argument constructors explicit, just to prevent future errors. For example, imagine if later on, you added the casting operator for RGBA-to-CMYK to the CMYK class. What, then, would color = Color(some_cmyk); call? Technically, it could call either Color(const CMYK&), which we'd expect, or Color(const RGBA&), because CMYK can be implicitly cast. This unpredictability is not something we want to deal with. To get around this, you can mark many of your constructors as explicit, to prevent implicit casting. explicit Color(const RGBA& rgba) // ... explicit Color(const CMYK& cmyk) // ... Then, calling Color(some_cmyk) would only be able to fit the call signature of Color(const CMYK&); no implicit cast would be attempted. | |
| anari-source/include/anari/colors_test.hpp | ||
| 57 ↗ | (On Diff #1186) | Remove this comment throughout before landing this code. Comments that get committed should only relate to your specific code; you don't want to duplicate the documentation for Goldilocks usage. |
| 66–77 ↗ | (On Diff #1186) | Be sure you remove these comments before this code lands! |
| 143 ↗ | (On Diff #1186) | ...remove this comment throughout... (I'll let you find the other instances). |
| anari-source/src/colors.cpp | ||
| 74 | You still need to replace this comment with your answer to this question. | |
Comment Actions
Build is green
Building Anari on mpm-bionic-000fhoarro8rs as build 78.
See https://jenkins.mousepawmedia.net:8449/job/anari_build/78/ for more details.
Comment Actions
Build is green
Building Anari on mpm-bionic-000i3txpilgmq as build 82.
See https://jenkins.mousepawmedia.net:8449/job/anari_build/82/ for more details.
Comment Actions
Good changes!
Be sure to mark my inline comments as Done before this can be landed. Keep in mind the Differential Checklist:
Comment Actions
just wanted to talk about a few things about what to do which direction to take because i was lost on my own
Comment Actions
I know how that feels. We should schedule a meeting this week anyway to catch up, so we can discuss that then. What days/times are you available the rest of this week?
Comment Actions
Sorry my internet has been acting up a lot this week. Maybe Saturday night regular time will be fine by me if it's ok with you.
Comment Actions
Build is green
Building Anari on mpm-bionic-000908t79oen2 as build 90.
See https://jenkins.mousepawmedia.net:8449/job/anari_build/90/ for more details.
Comment Actions
Build has FAILED
Building Anari on mpm-bionic-0000zgv05rfnu as build 105.
Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/106/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/106/console
Comment Actions
Build has FAILED
Building Anari on mpm-bionic-0000zgv05rfnu as build 105.
Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/107/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/107/console
Comment Actions
Build has FAILED
Building Anari on mpm-bionic-0000zgv05rfnu as build 105.
Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/108/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/108/console
Comment Actions
Build has FAILED
Building Anari on mpm-bionic-0000zgv05rfnu as build 105.
Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/109/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/109/console
Comment Actions
Build is green
Building Anari on mpm-bionic-0000uuej4p41e as build 110.
See https://jenkins.mousepawmedia.net:8449/job/anari_build/110/ for more details.
Comment Actions
A few notes about your tests, but you're overall on the right track! Get the rest of your tests and docs written, and you're golden!
| anari-source/include/anari/colors_test.hpp | ||
|---|---|---|
| 324 ↗ | (On Diff #1276) | I don't see where you use this? |
| 328 ↗ | (On Diff #1276) | I wouldn't do it this way. Since you're literally testing the RGBA constructor, you should only call said constructor in the context of run(). Think about how this test would run if the constructor were accidentally assigning red to RGBA.blue: your test as written would never detect that and fail (although it should), because it only knows that RGBA.red == RGBA.red, which is always true anyway on basis of variable identity. Part of the trick with testing is to isolate what you're trying to test, and make sure it will fail if anything but the right behavior occurs. Here's a good way to rewrite the test: public: Test_RGBAConstructor() : red(10), green(20), blue(30), alpha(255) {} // ... bool run() override { RGBA rgba_test = RGBA(red, green, blue, alpha); PL_ASSERT_EQUAL(rgba_test.red, red); PL_ASSERT_EQUAL(rgba_test.green, green); PL_ASSERT_EQUAL(rgba_test.blue, blue); PL_ASSERT_EQUAL(rgba_test.alpha, alpha); } // ... Make sense? Make sure that you isolate what function and/or behavior you're testing, and keep that contained in test() (and pre()/janitor() as appropriate). |
| 363 ↗ | (On Diff #1276) | Same issues here, and with the other constructor tests. |
Comment Actions
Build is green
Building Anari on mpm-bionic-0004awgf8e52a as build 116.
See https://jenkins.mousepawmedia.net:8449/job/anari_build/116/ for more details.
Comment Actions
Very nearly there! One typo and one more section of documentation, and I believe you can call this done!
| docs/source/attributes/colors.rst | ||
|---|---|---|
| 18 ↗ | (On Diff #1292) | HSV? |
| 66 ↗ | (On Diff #1292) | "Hex" is short for "Hexadecimal", rather than being an acronym like "RGBA (Red Green Blue Alpha)". Therefore, it shouldn't be in all-caps. |
Comment Actions
Build is green
Building Anari on mpm-bionic-000gfqoqtvyqj as build 117.
See https://jenkins.mousepawmedia.net:8449/job/anari_build/117/ for more details.
Comment Actions
While you're planning next steps for Anari, I'm having @gacmix review this to familiarize himself with the Anari source so far, as well as C++ in general. I'll leave it open for this week to give him a chance to do that; once he approves as well, we'll land it!
Comment Actions
Build has FAILED
Building Anari on mpm-bionic-000kfkw5stox7 as build 118.
Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/118/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/118/console
Comment Actions
Build has FAILED
Building Anari on mpm-bionic-0001xbc15qehn as build 119.
Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/119/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/119/console
Comment Actions
Build is green
Building Anari on mpm-bionic-0003k06ktj91o as build 120.
See https://jenkins.mousepawmedia.net:8449/job/anari_build/120/ for more details.
Comment Actions
Confirmed green on all tests on my machine, for both Debug and Release. Let's land this puppy!
@mahussain, run arc land on your branch to land it to master.
Comment Actions
Build is green
Building Anari on mpm-bionic-0005kmhr36ovv as build 121.
See https://jenkins.mousepawmedia.net:8449/job/anari_build/121/ for more details.
Comment Actions
Oh! You were blocked because you updated after the approval, so the approval expired. That's normal. I've re-approved, so you should be able to land it now.