Skip to content
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

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cmake/f3dOptions.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ function(_parse_json_option _top_json)
elseif(_option_type STREQUAL "ratio")
set(_option_actual_type "f3d::ratio_t")
set(_option_variant_type "double")
elseif(_option_type STREQUAL "vector")
set(_option_actual_type "f3d::vector3_t")
set(_option_variant_type "std::vector<double>")
set(_option_default_value_start "{")
set(_option_default_value_end "}")
endif()

# Add option to struct and methods
Expand Down
4 changes: 4 additions & 0 deletions library/options.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
"type": "string",
"default_value": "+Y"
},
"up_direction2": {
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

"type": "vector",
"default_value": "0, 1, 0"
},
"animation": {
"autoplay": {
"type": "bool",
Expand Down
89 changes: 89 additions & 0 deletions library/private/options_tools.h.in
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
#include "types.h"

#include <algorithm>
#include <cctype>
#include <exception>
#include <ios>
#include <iostream>
#include <sstream>
#include <vector>
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved

namespace f3d
{
Expand Down Expand Up @@ -181,6 +186,79 @@ ratio_t parse(const std::string& str)
}
}

//----------------------------------------------------------------------------
/**
* Parse provided string into a vector3_t.
*/
template<>
vector3_t parse(const std::string& str)
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved
{
auto parseCommaSeparated = [](const std::string& str) -> vector3_t
{
std::vector vec = parse<std::vector<double>>(str);
if (vec.size() == 2)
{
return f3d::vector3_t::fromSphericalCoordinates(vec[0], vec[1]);
}
if (vec.size() == 3)
{
return { vec[0], vec[1], vec[2] };
}
else
{
throw options::parsing_exception("cannot parse " + str + " to a vector3_t");
}
};
auto parseYSyntax = [](const std::string& str) -> vector3_t
{
auto ss = trim(str);
if (ss.size() != 2)
{
throw options::parsing_exception("cannot parse " + str + " to a vector3_t");
}
int sign = +1;
Copy link
Contributor

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:

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.

Copy link
Contributor

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.

if (ss[0] == '-')
{
sign = -1;
}
else if (ss[0] == '+')
{
sign = +1;
}
else
{
throw options::parsing_exception("cannot parse " + str + " to a vector3_t");
}
if (ss[1] == 'X')
{
return { double(sign) * 1, 0, 0 };
}
else if (ss[1] == 'Y')
{
return { 0, double(sign) * 1, 0 };
}
else if (ss[1] == 'Z')
{
return { 0, 0, double(sign) * 1 };
}
else
{
throw options::parsing_exception("cannot parse " + str + " to a vector3_t");
}
};
bool isNotCommaSeparated = std::find_if(str.begin(), str.end(),
[](unsigned char c) { return std::isalpha(c); }) != str.end();
std::cout << isNotCommaSeparated << '\n';
if (!isNotCommaSeparated)
{
return parseCommaSeparated(str);
}
else
{
return parseYSyntax(str);
}
}

//----------------------------------------------------------------------------
/**
* Return provided string stripped of leading and trailing spaces.
Expand Down Expand Up @@ -270,6 +348,17 @@ std::string format(const std::vector<double>& var)
return stream.str();
}

//----------------------------------------------------------------------------
/**
* Format provided var into a string from provided vector3_t
* rely on format(double&)
*/
template<>
std::string format(const vector3_t& var)
mwestphal marked this conversation as resolved.
Show resolved Hide resolved
{
return format(static_cast<std::vector<double>>(var));
}

//----------------------------------------------------------------------------
/**
* Generated method, see `options::set`
Expand Down
142 changes: 130 additions & 12 deletions library/public/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#include "export.h"

#include <array>
#include <cmath>
#include <initializer_list>
#include <stdexcept>
#include <string>
#include <vector>

Expand All @@ -21,18 +24,6 @@ struct F3D_EXPORT point3_t : std::array<double, 3>
}
};

/**
* Describe a 3D vector.
*/
struct F3D_EXPORT vector3_t : std::array<double, 3>
{
template<typename... Args>
vector3_t(Args&&... args)
: array({ double(std::forward<Args>(args))... })
{
}
};

/**
* Describe an angle in degrees.
*/
Expand Down Expand Up @@ -82,6 +73,133 @@ struct mesh_t
*/
F3D_EXPORT std::pair<bool, std::string> isValid() const;
};

/**
* Describe a 3D vector.
*/
struct F3D_EXPORT vector3_t
{
vector3_t() = default;
vector3_t(double x, double y, double z)
: Value{ x, y, z }
{
}
vector3_t(const std::vector<double>& vec)
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved
{
if (vec.size() != 3)
{
throw std::runtime_error("cannot create a vector3_t");
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved
}
Value[0] = vec[0];
Value[1] = vec[1];
Value[2] = vec[2];
}
Copy link
Contributor

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

vector3_t(const std::array<double, 3>& arr)
{
Value[0] = arr[0];
Value[1] = arr[1];
Value[2] = arr[2];
}
vector3_t(double* ptr)
{
Value[0] = ptr[0];
Value[1] = ptr[1];
Value[2] = ptr[2];
}
vector3_t(std::initializer_list<double> l)
{
if (l.size() != 3)
{
throw std::runtime_error("cannot create a vector3_t");
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved
}
std::copy(l.begin(), l.end(), std::begin(Value));
}

double* data()
{
return Value;
}
const double* data() const
{
return Value;
}

double& operator[](int idx)
{
return Value[idx];
}
double operator[](int idx) const
{
return Value[idx];
}
operator std::vector<double>() const
{
return { Value[0], Value[1], Value[2] };
}
operator std::array<double, 3>() const
{
return { Value[0], Value[1], Value[2] };
}
bool operator==(const vector3_t& vec) const
{
return Value[0] == vec.Value[0] && Value[1] == vec.Value[1] && Value[2] == vec.Value[2];
}
bool operator!=(const vector3_t& vec) const
{
return !(*this == vec);
}

double* begin()
{
return Value;
}
const double* begin() const
{
return Value;
}
const double* cbegin() const
{
return Value;
}
double* end()
{
return Value + 3;
}
const double* end() const
{
return Value + 3;
}
const double* cend() const
{
return Value + 3;
}

static vector3_t fromSphericalCoordinates(double theta, double phi)
mwestphal marked this conversation as resolved.
Show resolved Hide resolved
{
auto sinPhi = std::sin(phi);
auto cosTheta = std::cos(theta);
return { sinPhi * cosTheta, sinPhi * cosTheta, std::cos(phi) };
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved
}
static vector3_t x()
{
return { 1, 0, 0 };
}
static vector3_t y()
{
return { 0, 1, 0 };
}
static vector3_t z()
{
return { 0, 0, 1 };
}
static vector3_t zero()
{
return { 0, 0, 0 };
}

private:
double Value[3];
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved
};
}

#endif
2 changes: 2 additions & 0 deletions library/src/options.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "export.h"
#include "init.h"
#include "log.h"
#include "types.h"
#include "utils.h"

#include "vtkF3DConfigure.h"
Expand Down Expand Up @@ -178,6 +179,7 @@ F3D_DECL_TYPE(bool);
F3D_DECL_TYPE(int);
F3D_DECL_TYPE(double);
F3D_DECL_TYPE(f3d::ratio_t);
F3D_DECL_TYPE(f3d::vector3_t);
F3D_DECL_TYPE(std::string);

//----------------------------------------------------------------------------
Expand Down
13 changes: 13 additions & 0 deletions library/testing/PseudoUnitTest.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef PseudoUnitTest_h
#define PseudoUnitTest_h

#include "types.h"
Copy link
Contributor

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

Copy link
Contributor

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?

#include <functional>
#include <iostream>
#include <sstream>
Expand Down Expand Up @@ -129,6 +130,18 @@ class PseudoUnitTest
return ss.str();
}

std::string toString(const f3d::vector3_t& value)
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved
{
std::stringstream ss;
size_t i = 0;
for (const double& item : value)
{
ss << (i++ ? ", " : "{ ") << this->toString(item);
}
ss << " }";
return ss.str();
}

protected:
template<typename T1, typename T2>
std::string comparisonMessage(const T1& actual, const T2& expected, const std::string& comp)
Expand Down
12 changes: 12 additions & 0 deletions library/testing/TestSDKOptionsIO.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include <options.h>

#include "PseudoUnitTest.h"
#include "types.h"

#include <iostream>

Expand Down Expand Up @@ -71,5 +72,16 @@ int TestSDKOptionsIO(int argc, char* argv[])
test.parse<std::vector<std::string>>(
"std::vector<std::string>", " foo, bar , baz ", { "foo", "bar", "baz" });

test.parse<f3d::vector3_t>("vector3_t", "1, 2, 3", { 1, 2, 3 });
test.parse<f3d::vector3_t>("vector3_t", " 1, 2, 3 ", { 1, 2, 3 });
test.parse<f3d::vector3_t>("vector3_t", "+Y", { 0, 1, 0 });
test.parse<f3d::vector3_t>("vector3_t", " +Y ", { 0, 1, 0 });
test.parse<f3d::vector3_t>("vector3_t", "-Y", { 0, -1, 0 });
test.parse<f3d::vector3_t>("vector3_t", "+X", { 1, 0, 0 });
test.parse<f3d::vector3_t>("vector3_t", "-X", { -1, 0, 0 });
test.parse<f3d::vector3_t>("vector3_t", "+Z", { 0, 0, 1 });
test.parse<f3d::vector3_t>("vector3_t", "-Z", { 0, 0, -1 });
test.parse<f3d::vector3_t>("vector3_t", "1, 2", f3d::vector3_t::fromSphericalCoordinates(1, 2));
Yogesh9000 marked this conversation as resolved.
Show resolved Hide resolved

return test.result();
}
Loading