Vector3 to vector4 functions#132
Conversation
Signed-off-by: Lucas Fernando <lucas.costa@ee.ufcg.edu.br>
Signed-off-by: Lucas Fernando <lucas.costa@ee.ufcg.edu.br>
|
ignition_math-abichecker-any_to_any-ubuntu_auto-amd64 and ignition_math-ci-pr_any-ubuntu_auto-amd64 do not pass because they depends on the "Vector4.i" file, that I suppose to be an interface to ruby test code, and both does not exist on branch ign-math6 for Vector4. I have started to write both "Vector4.i" and "Vector4_TEST.rb", the ideal is report this in an new Issue and make a new PR? |
I believe if you want you can leave |
mjcarroll
left a comment
There was a problem hiding this comment.
Some suggestions for conciseness, and a few vestigial changes.
include/ignition/math/Vector4.hh
Outdated
| /// \return the maximum element | ||
| public: T Max() const | ||
| { | ||
| return std::max(std::max(std::max(this->data[0], |
There was a problem hiding this comment.
I believe that std::max_element would be a bit more concise: https://en.cppreference.com/w/cpp/algorithm/max_element
include/ignition/math/Vector4.hh
Outdated
| /// \return the minimum element | ||
| public: T Min() const | ||
| { | ||
| return std::min(std::min(std::min(this->data[0], |
There was a problem hiding this comment.
Same as above but with std::min_element.
include/ignition/math/Vector4.hh
Outdated
| if (_v[0] > this->data[0]) | ||
| this->data[0] = _v[0]; |
There was a problem hiding this comment.
I believe this would be more concise
| if (_v[0] > this->data[0]) | |
| this->data[0] = _v[0]; | |
| this->data[0] = std::max(_v[0], this->data[0]); |
There was a problem hiding this comment.
I thing the same, and prefer std::min, only did with the ifs because is implemented this way in Vector3. Despite that I can use std::min?
There was a problem hiding this comment.
I prefer this, it would actually probably make sense to go back and fix vector3 and vector2 to be the same, but it's out of the scope of this pr.
| /// \brief Set this vector's components to the minimum of itself and the | ||
| /// passed in vector | ||
| /// \param[in] _v the minimum clamping vector | ||
| public: void Min(const Vector4<T> &_v) |
src/Vector4_TEST.cc
Outdated
| #include "ignition/math/Helpers.hh" | ||
| #include "ignition/math/Matrix4.hh" | ||
| #include "ignition/math/Vector4.hh" | ||
| #include "../include/ignition/math/Vector4.hh" |
There was a problem hiding this comment.
This doesn't seem necessary, the same header should be included above.
There was a problem hiding this comment.
Ops, I forgot that rs, will remove.
include/ignition/math/RollingMean.hh
Outdated
| #define IGNITION_MATH_ROLLINGMEAN_HH_ | ||
|
|
||
| #include <memory> | ||
| #include <limits> |
There was a problem hiding this comment.
I agree and will remove from the PR, only put this library because before it the build is giving the following error:
ign-math/src/RollingMean.cc:57:10: error: 'numeric_limits' is not a member of 'std'
Must I create an Issue for this?
There was a problem hiding this comment.
It's fine to put in here, but since it's used in the cc file, rather than the header, it should be added there, I believe.
Signed-off-by: Lucas Fernando <lucas.costa@ee.ufcg.edu.br>
Signed-off-by: Lucas Fernando <lucas.costa@ee.ufcg.edu.br>
… vector3_to_vector4_functions
Signed-off-by: Lucas Fernando <lucas.costa@ee.ufcg.edu.br>
Remove Also had finished |
luccosta
left a comment
There was a problem hiding this comment.
Changes made, only in doubt with the proper use of std::max_element with pure arrays.
mjcarroll
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for the contribution!
Resolves #71 .
This implements the functions Min(), Min(Vector4), Max() and Max(Vector4) in Vector4 API, similar to the existing same functions in Vector3.
It enables Vector4 objects to perform:
vec4_a.max(vec4_b); vec4_a.min(vec4_b); vec4.max(); vec4.min();All the tests passed, and I dont know how to evaluate the code coverage.
To build the changes correctly i had to adjust some files.
One was src/CMakeLists.txt to add Vector4 to the swig files, because the include of Vector4 library at Vector4_TEST.cc was using build/fake/install/include/ignition/math7/ignition/math/Vector4.hh. Also had to change RollingMean.hh because the lack of the limits library was causing build errors.
This is my first pull request, I was in doubt if was better make the PR with this adjusts asking if they were correct or ask about them before make the PR and if the correct thing to do is to make this adjusts in another PR. Also, I would like to receive advices to improvement.