-
-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor vector3_t #1645 #1648
base: master
Are you sure you want to change the base?
Refactor vector3_t #1645 #1648
Conversation
You are modifying libf3d public API! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1648 +/- ##
==========================================
- Coverage 96.85% 96.80% -0.05%
==========================================
Files 108 104 -4
Lines 7691 7861 +170
==========================================
+ Hits 7449 7610 +161
- Misses 242 251 +9 ☔ View full report in Codecov by Sentry. |
@@ -4,6 +4,10 @@ | |||
"type": "string", | |||
"default_value": "+Y" | |||
}, | |||
"up_direction2": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a test in TestSDKOption.cxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what am I supposed to here exactly. there doesn't seems to be test for other options in TestSDKOption.cxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestSDKOptions.cxx test all types of options, you are adding a new type of option, you should test it.
{ | ||
throw options::parsing_exception("cannot parse " + str + " to a vector3_t"); | ||
} | ||
int sign = +1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have a logic based on regex working there:
f3d/vtkext/private/module/vtkF3DRenderer.cxx
Line 236 in 33771c5
if (std::regex_match(upString, match, re)) |
Feel free to copy it.
The advantage is it supports lowercase, omitting the
+
prefix, and errors out if invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yogesh9000 please take this into account.
Co-authored-by: snoyer <[email protected]>
@Meakk @mwestphal have you reviewed the changes fo this pr |
*/ | ||
struct F3D_EXPORT point3_t : std::array<double, 3> | ||
struct type_creation_exception : public exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing F3D_EXPORT I think
Value[0] = vec[0]; | ||
Value[1] = vec[1]; | ||
Value[2] = vec[2]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all above should be covered by tests
{ | ||
os << "{ " << vec[0] << ", " << vec[1] << ", " << vec[2] << " }"; | ||
return os; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above should be covered by tests
@@ -1,6 +1,7 @@ | |||
#ifndef PseudoUnitTest_h | |||
#define PseudoUnitTest_h | |||
|
|||
#include "types.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this would be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume it's to know about std::ostream& operator<<(std::ostream& os, const f3d::vector3_t& vec)
.
I think it would be better if it was not included from inside PseudoUnitTest.h
(to keep that bit of code as "standalone" as possible) but I don't know if it's doable. Would it work to have #include "types.h"
before #include "PseudoUnitTest.h"
in TestSDKOptionsIO.cxx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed, some changes needed.
BTW you will need to try and replace up_direction2 into up_direction (and make the appropreiate changes) before being able to merge.
Please keep these changes into a separate commit for clarity.
If needed I can take over to do it, do not hesitate :)
refactored vector3_t and added a naive parse function