Page MenuHomePhabricator

Transformation and LayerInstance classes
ClosedPublic

Authored by gacmix on Sep 10 2020, 9:57 PM.

Details

Summary

Classes that represent linear matrices transformations and layer instances

Test Plan

Refer to their seperate test files

Diff Detail

Repository
rA Anari
Lint
Lint Not Applicable

Event Timeline

Build has FAILED

Building Anari on mpm-bionic-0002xeu42448u as build 195.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/195/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/195/console

Restricted Application returned this revision to the author for changes because remote builds failed.Sep 10 2020, 9:57 PM
Restricted Application failed remote builds in Restricted Buildable!
  • Added license comments
  • Merge branch 'devel' into TransformationClass
  • Added license to layerinstance

Build has FAILED

Building Anari on mpm-bionic-0002xl0x75kha as build 196.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/196/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/196/console

Restricted Application returned this revision to the author for changes because remote builds failed.Sep 10 2020, 10:06 PM
Restricted Application failed remote builds in Restricted Buildable!

Build has FAILED

Building Anari on mpm-bionic-0002xmauyzrfg as build 197.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/197/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/197/console

Restricted Application returned this revision to the author for changes because remote builds failed.Sep 10 2020, 10:07 PM
Restricted Application failed remote builds in Restricted Buildable!
  • Created layerinstance_tests.hpp

Build has FAILED

Building Anari on mpm-bionic-00046yjeaevfd as build 198.

Link to build: https://jenkins.mousepawmedia.net:8449/job/anari_build/198/
See console output for more information: https://jenkins.mousepawmedia.net:8449/job/anari_build/198/console

Restricted Application failed remote builds in Restricted Buildable!Sep 12 2020, 9:40 AM

Here's what I found. A few style changes, one or two errors, and some suggested improvements.

Also, you can find out why the Jenkins build is failing by clicking on the link provided after See console output for more information: The easiest way to read that output is to scroll to the bottom, and work your way backwards. In this case, it's because of the IOChannel error we already fixed. I'll see if I can update the central PawLIB build to resolve that.

anari-source/include/anari/layerinstance.hpp
35–36

You can dedent this sector a level. We usually put public:, protected: and private: on the same level as class to save on indentation.

42

If you skip new, this will be created on the heap instead of the stack, which appears to be what you're going for. Dereferencing and assigning to a member variable like this copies from the heap allocated object onto a stack allocated object, meaning the dynamic allocation itself was wasted effort.

Otherwise, if you want this to live on the heap, make the transform member variable into a std::shared_ptr<Transform>.

Also, we usually avoid inline comments, in favor of putting them above the line instead. This is so we don't make the code too wide. You'll want to adjust that throughout.

56

This would be an error, as transform is type Transformation, not LayerInstance (the type of cpy). I think you meant this...

65
73–75

For the sake of consistency, you should choose one of these two names for use in both classes: either perform_transformation or do_transformation. When choosing function names, especially for public member functions, you want to minimize the learning curve and memory load for the user. Always assume someone else will be using your class(es).

anari-source/include/anari/transformation.hpp
35–42

One thing you could do to ensure the matrix chosen always matches DIMENSION is to put a typedef right here. That will save you further refactoring if you ever need to move from Matrix3d or something else. (It also reduces the possibility of error.)

If you do this, be sure to change Matrix3d to TransformMatrix throughout.

This revision now requires changes to proceed.Sep 12 2020, 12:54 PM

Build is green

Building Anari on mpm-bionic-0004b5yfqu5e6 as build 199.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/199/ for more details.

gacmix marked 5 inline comments as done.
  • Incorporated Jason's changes

Build is green

Building Anari on mpm-bionic-00081ftxmcffd as build 200.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/200/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Sep 16 2020, 10:10 PM

Build is green

Building Anari on mpm-bionic-00081ncensk5h as build 201.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/201/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Sep 16 2020, 10:20 PM

Nice!

I overlooked a detail in one of my earlier suggestions. Besides this, you should be about ready to land. If my last suggestion doesn't work, just let me know and I'll approve this as-is.

anari-source/include/anari/layerinstance.hpp
40–43

Oh. Ha, I just spotted the initialization list. My mistake! That can actually be used for this ....I think. You'll have to double-check that the tests still pass with this change.

This revision now requires changes to proceed.Sep 17 2020, 6:14 AM
ardunster added inline comments.
anari-source/include/anari/layerinstance_tests.hpp
10

Was just looking through this out of curiosity and noticed this. I think you meant this spelling. :)

gacmix marked an inline comment as done.
  • Finalized the changes

Build is green

Building Anari on mpm-bionic-0009sucw8n2oz as build 205.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/205/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Sep 18 2020, 11:52 PM
  • Created layerinstance_tests.hpp
  • Ran tests, added minor changes

Build is green

Building Anari on mpm-bionic-0009tcekxv871 as build 206.

See https://jenkins.mousepawmedia.net:8449/job/anari_build/206/ for more details.

Restricted Application completed remote builds in Restricted Buildable.Sep 19 2020, 12:15 AM

Everything is ready to go, just need your approval and this revision can be landed! @jcmcdonald @mahussain

This revision is now accepted and ready to land.Sep 19 2020, 7:27 AM
This revision was automatically updated to reflect the committed changes.
gacmix marked an inline comment as done.