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

Precision must be double for JSON #528

Merged
merged 1 commit into from
Aug 22, 2018
Merged

Conversation

tribal-tec
Copy link
Contributor

No description provided.

@@ -37,29 +37,29 @@ class Transformation : public BaseObject
public:
Transformation() = default;

Transformation(const Vector3f& translation, const Vector3f& scale,
const Quaternionf& rotation, const Vector3f& rotationCenter)
Transformation(const Vector3d& translation, const Vector3f& scale,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the scale and rotationCenter also use Vector3d?

Quaternionf _rotation;
Vector3f _rotationCenter{0.f, 0.f, 0.f};
Vector3d _translation{0., 0., 0.};
Vector3d _scale{1.f, 1.f, 1.f};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove .f?

Vector3d _translation{0., 0., 0.};
Vector3d _scale{1.f, 1.f, 1.f};
Quaterniond _rotation;
Vector3d _rotationCenter{0.f, 0.f, 0.f};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove .f?

@@ -93,7 +93,7 @@ inline Transformation operator*(const Transformation& a,
a.getRotationCenter()};
}

inline Boxf transformBox(const Boxf& box, const Transformation& trafo)
inline Boxd transformBox(const Boxd& box, const Transformation& trafo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's trafo? 😄

tests/brayns.cpp Outdated

const auto& animParams = pm.getAnimationParameters();
BOOST_CHECK_EQUAL(animParams.getFrame(), 0);

const auto& volumeParams = pm.getVolumeParameters();
BOOST_CHECK_EQUAL(volumeParams.getDimensions(), brayns::Vector3ui(0, 0, 0));
BOOST_CHECK_EQUAL(volumeParams.getElementSpacing(),
brayns::Vector3f(1.f, 1.f, 1.f));
brayns::Vector3d(1.f, 1.f, 1.f));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove .f?

tests/brayns.cpp Outdated
BOOST_CHECK_EQUAL(volumeParams.getOffset(),
brayns::Vector3f(0.f, 0.f, 0.f));
brayns::Vector3d(0.f, 0.f, 0.f));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove .f?

@tribal-tec tribal-tec force-pushed the precision branch 2 times, most recently from b2ca871 to 278ebf4 Compare August 22, 2018 08:07
@tribal-tec tribal-tec requested a review from karjonas August 22, 2018 08:19
JSON comes with infinite precision and if incoming numbers are truncated to
float, we can not guarantee to re-broadcast the same numbers again.
Float is still supported nonetheless in general, but no longer for PropertyMap.
@@ -33,6 +33,15 @@ namespace brayns
ospSet##OSP(ospObject, prop->name.c_str(), \
prop->get<std::array<TYPE, NUM>>().data()); \
break;
#define SET_ARRAY_FLOAT(OSP, NUM) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could make these defines templated functions. For another PR though.

@tribal-tec tribal-tec merged commit e5e10e8 into BlueBrain:master Aug 22, 2018
@tribal-tec tribal-tec deleted the precision branch August 22, 2018 08:52
@tribal-tec tribal-tec added the bug label Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants