Page MenuHomePhabricator

Transformation Refactor
Needs ReviewPublic

Authored by wdede on Oct 16 2020, 11:54 PM.

Details

Summary

Refactor the transformation class to accept dynamic arrays

Test Plan

Write test classes

Diff Detail

Repository
rA Anari
Branch
arcpatch-D354
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/transformationrefactored.hpp
34 β†—(On Diff #1636)

That was probably because @gacmix was writing the copy assignment and referring to the previous version of Transformation and I saw the copy constructor and said he needed both.... Without realizing it was effectively the same as the constructor he already had with a Transformation as an argument πŸ€¦β€β™€οΈ

anari-source/include/anari/transformationrefactored.hpp
34 β†—(On Diff #1636)

The good ol' autopilot!

memateo requested changes to this revision.Oct 29 2020, 7:51 PM

The two constructors that I commented about, rest looks good!

@memateo You should Commandeer this revision. From the bottom of the page, go to Add Action... β†’ Commandeer Revision. This will make you the author, and give you complete control over the diff, as if it were your own.

memateo edited reviewers, added: β€’ gacmix; removed: memateo.

Removed duplicate constructor

Restricted Application completed remote builds in Restricted Buildable.Nov 21 2020, 12:25 AM
  • This time, actually added the changes
Restricted Application completed remote builds in Restricted Buildable.Nov 21 2020, 12:28 AM

Was looking to see if this was ready to approve yet but there's no tests so I guess not? Probably just needs to pass similar tests as the original Transformation class. I know Graham had planned to delete the original class and replace it with this one once it was all in working order.

anari-source/include/anari/transformation.hpp
9–10

I'm going to go on a limb and suggest this happened as a result of random typing glitching during a liveshare session....

anari-source/include/anari/transformationrefactored.hpp
1 β†—(On Diff #1667)

This should have version and library and license info at the top like all the other header files, copypaste with a few edits should be fine. The ones in SIMPLEXpress have an edit date, too, not sure what the standard is on that.

  • Added tests, and a way to get values from the dynamic matrix member
Restricted Application completed remote builds in Restricted Buildable.Nov 29 2020, 2:23 PM
  • Fixed slight error and typo
Restricted Application completed remote builds in Restricted Buildable.Nov 29 2020, 2:24 PM
  • Fixed some warnings, and wrote an extra test
Restricted Application completed remote builds in Restricted Buildable.Dec 3 2020, 4:14 PM
jcmcdonald added inline comments.
anari-source/include/anari/transformationrefactored.hpp
6 β†—(On Diff #1696)

Our convention is to place names alphabetically by last name, principally to avoid debates about prominence of names.

36 β†—(On Diff #1696)

I don't recommend naming a class "Refactored". Just replace the old version. We're using Git, so you can always see the old versions.

45 β†—(On Diff #1696)

Can rows and columns ever be negative? If not, make the parameters unsigned int, or even better, size_t (an unsigned integer type used by most data structures for indices and lengths.)

47 β†—(On Diff #1696)

If you make the above change, you'll also want to change the type here from int to size_t.

Additionally, I recommend using prepended increment/decrement anytime the return value from the operation is not directly used, as ++i is one less CPU instruction than i++. The compiler usually optimizes this out anyway, but it's good to be in the habit for the rare scenario when it doesn't.

66 β†—(On Diff #1696)

You need a documentation comment on each function. Documentation comments are either /// ... or /** ... */. For normal functions (as opposed to constructors), you also need to describe all the parameters (\param) and the return value (\return), as appropriate.

67–68 β†—(On Diff #1696)

Comments inside functions should be ordinary comments //, not doc comments.

69 β†—(On Diff #1696)

Switch from spaces to tabs across all your files, please. Visual Studio Code can do this for you, and then clangformat can help with the rest of the formatting. Talk to @ardunster on how to do that.

1 β†—(On Diff #1667)

Yeah, we dropped the edit date convention, but there are still files that haven't been updated accordingly.

I agree with bringing the heading and license comments over though!

This revision now requires changes to proceed.Dec 12 2020, 1:39 PM
  • Replaces tabs with spaces (I hope), better documentation
Restricted Application completed remote builds in Restricted Buildable.Dec 20 2020, 2:48 PM
Restricted Application completed remote builds in Restricted Buildable.Dec 20 2020, 2:58 PM
anari-source/include/anari/colors.hpp
96–102

I know this is only visible because of running format, but I suspect that what was intended here was luminance, not luminace, given that that's not a real word.

It seems to me you only need to clear through the lint errors before we can land this. I've left a few comments to help you there, but you'll want to look at the rest of the lint errors and fix them. The only error type you should be suppressing with // cppcheck-suppress is constStatement β€” NONE of the others!

anari-source/include/anari/layerinstance_tests.hpp
52–53

Since this is syntactically correct with Eigen (apparently), you may need to silence the linter on this topic like this.

79

...and here.

You can find the rest.

86–89

This is rather odd to me. What actually happens if you combine these lines together? (I know this wasn't your code.)

This revision now requires changes to proceed.Dec 21 2020, 8:15 PM
  • Supressed a few errors, fixed some format, will need to look into layerinstance_tests
Restricted Application completed remote builds in Restricted Buildable.Jan 3 2021, 11:03 AM
Restricted Application completed remote builds in Restricted Buildable.Jan 3 2021, 11:06 AM
anari-source/include/anari/layerinstance_tests.hpp
86–89

It looks like it purposefully sets up a dummy layer instance, and then changes it to be a different one to test the copy constructor and make sure it works properly. Should I still change it?

Restricted Application completed remote builds in Restricted Buildable.Jan 3 2021, 11:12 AM
  • Luminace to Luminance (whoops)
Restricted Application completed remote builds in Restricted Buildable.Jan 3 2021, 11:14 AM
  • Fixed luminace typo in colors.cpp
Restricted Application completed remote builds in Restricted Buildable.Jan 10 2021, 1:17 PM
  • Fixed luminace typo in colors.cpp
  • Getting weird error pretaining to IoChannel and Goldilocks, fixed a few other things blocking the make
Restricted Application completed remote builds in Restricted Buildable.Jan 10 2021, 1:56 PM

I feel like this is worth landing if the tests are passing (as they apparently are). Further improvements can be made as needed, especially away from the formatting fixes.

Also, just checking, the contents of your .clang-format should be the same as in P56.

This revision is now accepted and ready to land.Jan 11 2021, 7:17 AM
  • Hopefully fix merge conflic
This revision now requires review to proceed.Feb 28 2021, 10:36 AM
Restricted Application completed remote builds in Restricted Buildable.Feb 28 2021, 10:36 AM

@memateo I don't want to lose all this work. What would it take to land this? Does it just need a review and approval from me?

wdede edited reviewers, added: memateo; removed: wdede.