Page MenuHomePhabricator

Color Class
ClosedPublic

Authored by mahussain on Nov 22 2019, 12:05 PM.

Details

Reviewers
jcmcdonald
gacmix
imirzali
Maniphest Tasks
T1247: ARGB Colors
Commits
rA08ff791afcf4: Color Class
Required Signatures
Restricted Legalpad Document
Summary

A class that represents colors in Anari and converts them in different formats

Test Plan

run color test suite (tbd)

Diff Detail

Repository
rA Anari
Branch
arcpatch-D248
Lint
Lint Passed

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
58

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.

67–78

Be sure you remove these comments before this code lands!

144

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

This revision now requires changes to proceed.Dec 18 2019, 8:14 AM
  • refined comments and linting errors

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.

Restricted Application completed remote builds in Restricted Buildable.Dec 18 2019, 9:11 PM
  • Added comments to colors.hpp

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.

Restricted Application completed remote builds in Restricted Buildable.Dec 21 2019, 10:58 PM

Good changes!

Be sure to mark my inline comments as Done before this can be landed. Keep in mind the Differential Checklist:

1Before landing, each Revision should...
2
3(1) Have an associated Maniphest Task.
4(2) Accomplish the feature(s) it was designed to accomplish. [In some cases, the feature itself may be dropped, and only bugfixes and/or optimizations landed instead.]
5(3) Have merged/rebased all changes from `devel` into itself, and all conflicts resolved. ($ git pull --rebase origin devel)
6(4) Have binaries and unnecessary cruft untracked and removed. (Keep an eye on .gitignore!)
7(5) Compile and run properly - this should be confirmed via Harbormaster/Jenkins (if available).
8(6) Be free of compiler errors and warnings (must compile with `-Wall -Wextra -Werror`).
9(7) Be Valgrind pure (no memory leaks detected).
10(8) Comply with Coding and Technical standards.
11(9) Be free of linter errors. ($ arc lint --lintall)
12(10) Be fully CSI commented.
13(11) Have an up-to-date build script (generally CMake) if relevant.
14(12) Contain relevant LIT tests, if the project is Goldilocks capable.
15(13) Have a Test Plan, generally containing a list of Goldilocks tests the reviewer should run.
16(14) Be reviewed, built, tested, and approved by at least one trusted reviewer (Staff or Trusted Contributor).
17(15) Have up-to-date Sphinx documentation, which compiles with no warnings.
18(16) Have all reviewer comments processed and marked "Done".

Hey Adeel, I understand you're a bit stuck with this. What's up?

Hey Adeel, I understand you're a bit stuck with this. What's up?

just wanted to talk about a few things about what to do which direction to take because i was lost on my own

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?

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?

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.

Works for me. I've created {E637}

  • Added comments to colors.cpp

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.

Restricted Application completed remote builds in Restricted Buildable.Jan 12 2020, 6:14 AM
  • Merge master
  • Added Constructors Test

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

Restricted Application failed remote builds in Restricted Buildable!Feb 23 2020, 4:46 PM

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

This failure seems to be in the pipeline. I'm going to restart the server.

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

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

Sorry for the noise. I'm trying to repair the build pipeline.

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.

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
325

I don't see where you use this?

329

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

364

Same issues here, and with the other constructor tests.

This revision now requires changes to proceed.Feb 24 2020, 5:53 PM
  • Changed the constructor tests
Restricted Application failed remote builds in Restricted Buildable!Mar 1 2020, 12:05 AM

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.

Restricted Application completed remote builds in Restricted Buildable.Mar 15 2020, 3:11 PM

Very nearly there! One typo and one more section of documentation, and I believe you can call this done!

docs/source/attributes/colors.rst
19

HSV?

67

"Hex" is short for "Hexadecimal", rather than being an acronym like "RGBA (Red Green Blue Alpha)". Therefore, it shouldn't be in all-caps.

This revision now requires changes to proceed.Mar 23 2020, 3:58 PM
  • Added RGBADouble to docs
  • Added HSL documentation

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.

Restricted Application completed remote builds in Restricted Buildable.Mar 29 2020, 9:31 PM

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!

This revision is now accepted and ready to land.Mar 30 2020, 4:01 PM

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

Restricted Application failed remote builds in Restricted Buildable!Apr 3 2020, 2:14 PM

I think this build fail is old. Ignore.

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

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.

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.

_landit_

This revision now requires review to proceed.Apr 11 2020, 1:30 AM

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.

Restricted Application completed remote builds in Restricted Buildable.Apr 11 2020, 1:32 AM
This revision is now accepted and ready to land.Apr 11 2020, 5:07 AM

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.

This revision was automatically updated to reflect the committed changes.
Restricted Application failed remote builds in Restricted Buildable!Apr 11 2020, 6:54 PM