Page MenuHomePhabricator
Differential D342

Geometry Refactor
ClosedPublic
Actions

Authored by memateo on Sep 16 2020, 10:24 PM.

Details

Reviewers
mahussain
jcmcdonald
ardunster
wdede
• gacmix
Maniphest Tasks
T1301: Refactoring Geometry Using Eigen
Commits
rAd6acb466413d: Geometry Refactor
Summary

Refactor the geometry classes so they use the Eigen library

Test Plan

Rewrite test classes

Diff Detail

Repository
rA Anari
Lint
Lint Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jenkins added a comment.Sep 16 2020, 10:25 PM

Build is green

Building Anari on mpm-bionic-00081ratp3161 as build 202.

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

• gacmix added a task: T1301: Refactoring Geometry Using Eigen.Sep 16 2020, 10:26 PM
jenkins added a comment.Sep 16 2020, 10:26 PM

Build is green

Building Anari on mpm-bionic-00081rrr31foc as build 203.
Running test suite A-sB03.

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

jenkins added a comment.Sep 16 2020, 10:26 PM

Build is green

Building Anari on mpm-bionic-00081s93md79d as build 204.
Running test suite A-sB01.

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

Restricted Application completed remote builds in Restricted Buildable.Sep 16 2020, 10:26 PM
• gacmix requested review of this revision.Sep 16 2020, 10:26 PM
• gacmix planned changes to this revision.Sep 18 2020, 12:56 PM
• gacmix updated this revision to Diff 1576.Sep 19 2020, 11:59 AM
  • Merge branch 'devel' of ssh://phabricator.mousepawmedia.net:2222/source/anari into GeometryRefactor
  • Added #include
jenkins added a comment.Sep 19 2020, 12:00 PM

Build is green

Building Anari on mpm-bionic-000a8ce8v3wnr as build 208.

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

jenkins added a comment.Sep 19 2020, 12:01 PM

Build is green

Building Anari on mpm-bionic-000a8d2k60rz8 as build 209.
Running test suite A-sB03.

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

jenkins added a comment.Sep 19 2020, 12:02 PM

Build is green

Building Anari on mpm-bionic-000a8dln8vg0a as build 210.
Running test suite A-sB01.

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

Restricted Application completed remote builds in Restricted Buildable.Sep 19 2020, 12:02 PM
• gacmix added a comment.Sep 19 2020, 12:02 PM

Will definitely need to plan out what changes need to occur in order to include the Eigen library to geometry.hpp and also need to see what needs to be cut out. Maybe a planning session @mahussain?

• gacmix planned changes to this revision.Sep 19 2020, 12:03 PM
• gacmix updated this revision to Diff 1577.Sep 21 2020, 1:56 PM
  • Refactored geometry.hpp and geometry_tests.hpp
jenkins added a comment.Sep 21 2020, 1:57 PM

Build has FAILED

Building Anari on mpm-bionic-000c02yc0rcd4 as build 211.

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

Restricted Application failed remote builds in Restricted Buildable!Sep 21 2020, 1:57 PM
• gacmix updated this revision to Diff 1578.Sep 21 2020, 1:57 PM
  • Added Eigen/Core
jenkins added a comment.Sep 21 2020, 1:57 PM

Build has FAILED

Building Anari on mpm-bionic-000c03i1wc4tp as build 212.

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

jenkins added a comment.Sep 21 2020, 1:58 PM

Build has FAILED

Building Anari on mpm-bionic-000c03tqqcbuk as build 213.

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

jenkins added a comment.Sep 21 2020, 1:58 PM

Build has FAILED

Building Anari on mpm-bionic-000c0451hmp5n as build 214.

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

Restricted Application failed remote builds in Restricted Buildable!Sep 21 2020, 1:58 PM
jenkins added a comment.Sep 21 2020, 1:59 PM

Build has FAILED

Building Anari on mpm-bionic-000c04ggxln6b as build 215.

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

jenkins added a comment.Sep 21 2020, 1:59 PM

Build has FAILED

Building Anari on mpm-bionic-000c04thdw8eu as build 216.

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

• gacmix planned changes to this revision.Sep 22 2020, 3:14 PM
• gacmix updated this revision to Diff 1585.Sep 28 2020, 11:27 AM
  • Fixed parentheses issue
  • Changed Vector4d -> Eigen::Vector4d
Restricted Application failed remote builds in Restricted Buildable!Sep 28 2020, 11:29 AM
jenkins added a comment.Sep 28 2020, 11:29 AM

Build has FAILED

Building Anari on mpm-bionic-0005rhq8klrip as build 217.

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

jenkins added a comment.Sep 28 2020, 11:29 AM

Build has FAILED

Building Anari on mpm-bionic-0005rikmew89q as build 218.

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

jenkins added a comment.Sep 28 2020, 11:30 AM

Build has FAILED

Building Anari on mpm-bionic-0005riwlm0vj6 as build 219.

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

• gacmix planned changes to this revision.Sep 30 2020, 2:22 PM
• gacmix updated this revision to Diff 1592.Oct 2 2020, 11:32 PM
  • Minor edits to test files
jenkins added a comment.Oct 2 2020, 11:33 PM

Build has FAILED

Building Anari on mpm-bionic-0009leb1bceka as build 220.

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

Restricted Application failed remote builds in Restricted Buildable!Oct 2 2020, 11:33 PM
jenkins added a comment.Oct 2 2020, 11:33 PM

Build has FAILED

Building Anari on mpm-bionic-0009let4dutpw as build 221.

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

jenkins added a comment.Oct 2 2020, 11:33 PM

Build has FAILED

Building Anari on mpm-bionic-0009lf5h0yifh as build 222.

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

• gacmix updated this revision to Diff 1593.Oct 2 2020, 11:36 PM
  • Switched vector names
jenkins added a comment.Oct 2 2020, 11:37 PM

Build has FAILED

Building Anari on mpm-bionic-0009lhtdg2zw2 as build 223.

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

Restricted Application failed remote builds in Restricted Buildable!Oct 2 2020, 11:37 PM
jenkins added a comment.Oct 2 2020, 11:37 PM

Build has FAILED

Building Anari on mpm-bionic-0009li5vz2zoy as build 224.

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

jenkins added a comment.Oct 2 2020, 11:38 PM

Build has FAILED

Building Anari on mpm-bionic-0009lii8h6now as build 225.

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

ardunster added a subscriber: ardunster.Oct 5 2020, 4:45 PM
ardunster added inline comments.
anari-source/include/anari/geometry_tests.hpp
82–99

Some of your build errors are because startVector and endVector are inside a limited scope, and they're called outside of it. Looking at it, it looks like this test was just to verify if the copy and the original curv object were the same (I can't remember why I didn't think so in our pair session), probably it would work if the second half of each line called copy instead?

120–128

These are just calling the wrong variable, probably a copypaste error. But I'm not sure why the test is ASSERT EQUAL if all it's trying to do is check if the subscript works. this may or may not produce the intended behavior.

• gacmix updated this revision to Diff 1600.Oct 6 2020, 3:26 PM
  • Refactored build errors in D342
• gacmix added a comment.Oct 6 2020, 3:27 PM

Thanks Anna, your changes seemed to help a lot!

jenkins added a comment.Oct 6 2020, 3:27 PM

Build is green

Building Anari on mpm-bionic-000cpkgtlq8q7 as build 227.

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

jenkins added a comment.Oct 6 2020, 3:27 PM

Build is green

Building Anari on mpm-bionic-000cpl2ht7jsd as build 228.
Running test suite A-sB03.

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

jenkins added a comment.Oct 6 2020, 3:28 PM

Build is green

Building Anari on mpm-bionic-000cplkng02yv as build 229.
Running test suite A-sB01.

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

Restricted Application completed remote builds in Restricted Buildable.Oct 6 2020, 3:28 PM
• gacmix updated this revision to Diff 1601.Oct 6 2020, 3:35 PM
  • Refactored Curve copy variable name
jenkins added a comment.Oct 6 2020, 3:36 PM

Build is green

Building Anari on mpm-bionic-000cprhhcygjl as build 230.

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

jenkins added a comment.Oct 6 2020, 3:37 PM

Build is green

Building Anari on mpm-bionic-000cps04xl3ei as build 231.
Running test suite A-sB03.

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

jenkins added a comment.Oct 6 2020, 3:37 PM

Build is green

Building Anari on mpm-bionic-000cpsib9wcoi as build 232.
Running test suite A-sB01.

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

Restricted Application completed remote builds in Restricted Buildable.Oct 6 2020, 3:37 PM
• gacmix added a comment.Oct 6 2020, 3:38 PM

Yeah so basically all of my build errors were because of incorrect variable names :/

wdede added a subscriber: wdede.Oct 6 2020, 3:41 PM
wdede added inline comments.
anari-source/include/anari/geometry_tests.hpp
90–98

Something about this is bugging me for some reason, I might be a bit confused by my Python notes but I want to make sure that this is the safest way to initialize or create a copy of the curv object here. I will research that and get back to you!

jcmcdonald added a comment.Oct 6 2020, 6:51 PM

Yeah so basically all of my build errors were because of incorrect variable names :/

Code blindness in a nutshell. Happens to the best!

jcmcdonald added inline comments.Oct 6 2020, 6:56 PM
anari-source/include/anari/geometry_tests.hpp
90–98

Ahh, nope, it's not. Good catch, @wdede.

As it stands, this code will call the default constructor, followed by the copy assignment operator, on Curve. That is, it's effectively...

Curve curv_copy();  // calls Curve::Curve() and allocates on stack
curv_copy = curve;  // calls Curve::operator=(const Curve&)

The better way to do this would be to use the copy constructor instead, like this:

Curve curv_copy(curve);  // calls Curve::Curve(const Curve&) and allocates on stack
wdede added inline comments.Oct 6 2020, 7:54 PM
anari-source/include/anari/geometry_tests.hpp
90–98

Thanks for the input @jcmcdonald, but I still don't fully get it. If we use Curve curv_copy(curve); the object curv is no longer needed, right??

jcmcdonald added inline comments.Oct 6 2020, 9:02 PM
anari-source/include/anari/geometry_tests.hpp
90–98

In this context, he's testing the copy constructor, so it's reasonable. He's testing the values of curv_copy against those in curv.

ardunster added a comment.EditedOct 7 2020, 12:58 PM
In D342#11701, @gacmix wrote:

Thanks Anna, your changes seemed to help a lot!

Glad I could help! :) It's mostly stuff I missed while we were doing the pair programming, too..... although I know you had somewhere to be right afterward so that's probably a factor.

• gacmix planned changes to this revision.Oct 11 2020, 6:16 PM
• gacmix updated this revision to Diff 1607.Oct 11 2020, 10:04 PM
• gacmix marked 6 inline comments as done.
  • Fixed copy constructor test
jenkins added a comment.Oct 11 2020, 10:05 PM

Build is green

Building Anari on mpm-bionic-0002uxd776iy8 as build 234.
Running test suite A-sB03.

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

jenkins added a comment.Oct 11 2020, 10:06 PM

Build is green

Building Anari on mpm-bionic-0002uy6roo3q3 as build 235.

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

jenkins added a comment.Oct 11 2020, 10:06 PM

Build is green

Building Anari on mpm-bionic-0002uyp7hb7ap as build 236.
Running test suite A-sB01.

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

Restricted Application completed remote builds in Restricted Buildable.Oct 11 2020, 10:06 PM
• gacmix added a comment.Oct 13 2020, 3:11 PM

I may need to refactor Transformation.hpp so that it can transform 4x1 Vectors

• gacmix added reviewers: ardunster, wdede, memateo.Oct 29 2020, 11:41 AM
ardunster accepted this revision.Nov 2 2020, 2:07 PM

To the best of my understanding of what's intended here (from the pair session I had with Graham and the couple times I have reviewed this code) it looks good, although I am not really familiar with how Eigen works/stores data so if there's an error there I don't think I would catch it. I'm going to accept this, I assume someone can still take ownership and land it?

This revision is now accepted and ready to land.Nov 2 2020, 2:07 PM
jcmcdonald added a comment.Nov 3 2020, 11:00 AM

@memateo You are welcome to Commandeer and land this.

memateo commandeered this revision.Nov 3 2020, 2:31 PM
memateo edited reviewers, added: • gacmix; removed: memateo.

TODO: Land

memateo mentioned this in Unknown Object (Ponder Question).Nov 4 2020, 6:03 PM
memateo mentioned this in Unknown Object (Ponder Answer).Nov 4 2020, 6:10 PM
Closed by commit rAd6acb466413d: Geometry Refactor (authored by • gacmix, committed by memateo). · Explain WhyNov 4 2020, 6:12 PM
This revision was automatically updated to reflect the committed changes.
Manuel Mateo <yekayama@gmail.com> added a commit: rAd6acb466413d: Geometry Refactor.
jcmcdonald awarded a token.Nov 4 2020, 6:13 PM

Revision Contents
Changeset List

PathSize
.vscode/
8 lines
anari-source/
include/
anari/
70 lines
56 lines
src/
6 lines
14 lines

Diff 1650

View Options

.vscode/settings.json

Loading...
View Options

anari-source/include/anari/geometry.hpp

Loading...
View Options

anari-source/include/anari/geometry_tests.hpp

Loading...
View Options

anari-source/src/test_renderer.cpp

Loading...
View Options

anari-source/src/uniform_renderer.cpp

Loading...
Copyright (C) 2019 MousePaw Media. All Rights Reserved. · Community Rules · Terms of Use · Privacy Policy · Terms of Development