Vector3 to vector4 functions #132

luccosta · 2020-07-13T14:12:01Z

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.

Signed-off-by: Lucas Fernando <[email protected]>

luccosta · 2020-07-13T16:50:58Z

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?

mjcarroll · 2020-07-14T16:39:45Z

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 believe if you want you can leave Vector4 out of the list of swig_files here: https://github.com/ignitionrobotics/ign-math/pull/132/files#diff-95e351a3805a1dafa85bf20b81d086e6R30, which should cause that generation to be skipped. We can then ticket that separately if you don't have it implemented right this second.

mjcarroll

Some suggestions for conciseness, and a few vestigial changes.

mjcarroll · 2020-07-14T16:41:26Z

include/ignition/math/Vector4.hh

+      /// \return the maximum element
+      public: T Max() const
+      {
+        return std::max(std::max(std::max(this->data[0],


I believe that std::max_element would be a bit more concise: https://en.cppreference.com/w/cpp/algorithm/max_element

mjcarroll · 2020-07-14T16:41:36Z

include/ignition/math/Vector4.hh

+      /// \return the minimum element
+      public: T Min() const
+      {
+        return std::min(std::min(std::min(this->data[0],


Same as above but with std::min_element.

Much better.

mjcarroll · 2020-07-14T16:43:28Z

include/ignition/math/Vector4.hh

+        if (_v[0] > this->data[0])
+          this->data[0] = _v[0];


I believe this would be more concise

Suggested change

if (_v[0] > this->data[0])

this->data[0] = _v[0];

this->data[0] = std::max(_v[0], this->data[0]);

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?

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.

mjcarroll · 2020-07-14T16:44:46Z

include/ignition/math/Vector4.hh

+      /// \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)


Same as above, but with std::min

mjcarroll · 2020-07-14T16:45:14Z

src/Vector4_TEST.cc

@@ -20,6 +20,7 @@
 #include "ignition/math/Helpers.hh"
 #include "ignition/math/Matrix4.hh"
 #include "ignition/math/Vector4.hh"
+#include "../include/ignition/math/Vector4.hh"


This doesn't seem necessary, the same header should be included above.

Ops, I forgot that rs, will remove.

mjcarroll · 2020-07-14T16:46:12Z

include/ignition/math/RollingMean.hh

@@ -18,6 +18,7 @@
 #define IGNITION_MATH_ROLLINGMEAN_HH_

 #include <memory>
+#include <limits>


Out of scope for this PR?

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?

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 <[email protected]>

… vector3_to_vector4_functions

Signed-off-by: Lucas Fernando <[email protected]>

luccosta · 2020-07-15T02:16:22Z

I believe if you want you can leave Vector4 out of the list of swig_files here: https://github.com/ignitionrobotics/ign-math/pull/132/files#diff-95e351a3805a1dafa85bf20b81d086e6R30, which should cause that generation to be skipped. We can then ticket that separately if you don't have it implemented right this second.

Remove Vector4 from swig_files and made the suggested changes.

Also had finished Vector4.i and am close to finish Vector4_TEST.rb, that will be a relative big pull request.

luccosta

Changes made, only in doubt with the proper use of std::max_element with pure arrays.

mjcarroll

Looks good to me, thanks for the contribution!

luccosta added 2 commits July 13, 2020 10:40

Add max and min functions to Vector4

690a66b

Signed-off-by: Lucas Fernando <[email protected]>

Include Vector4 in swig and limits library in RollingMean

ffc3d82

Signed-off-by: Lucas Fernando <[email protected]>

luccosta requested a review from scpeters as a code owner July 13, 2020 14:12

Merge branch 'ign-math6' into vector3_to_vector4_functions

856a8fc

chapulina requested a review from mjcarroll July 13, 2020 18:43

mjcarroll requested changes Jul 14, 2020

View reviewed changes

chapulina added enhancement New feature or request 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Jul 15, 2020

luccosta and others added 5 commits July 14, 2020 22:37

Changes to follow suggestions for conciseness

8632a94

Signed-off-by: Lucas Fernando <[email protected]>

Resolve vestigal changes

7079d7e

Signed-off-by: Lucas Fernando <[email protected]>

Merge remote branch 'vector3_to_vector4_functions' of fork into local…

7a09c1a

… vector3_to_vector4_functions

Remove Rolling Mean include

5d39745

Signed-off-by: Lucas Fernando <[email protected]>

Merge branch 'ign-math6' into vector3_to_vector4_functions

bad20fd

luccosta commented Jul 15, 2020

View reviewed changes

mjcarroll approved these changes Jul 20, 2020

View reviewed changes

mjcarroll merged commit 0270a51 into gazebosim:ign-math6 Jul 20, 2020

luccosta deleted the vector3_to_vector4_functions branch July 20, 2020 21:56

pxalcantara mentioned this pull request Jul 21, 2020

Add Max and Min function to Vector2.hh #133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vector3 to vector4 functions #132

Vector3 to vector4 functions #132

luccosta commented Jul 13, 2020 •

edited

Loading

luccosta commented Jul 13, 2020

mjcarroll commented Jul 14, 2020

mjcarroll left a comment

mjcarroll Jul 14, 2020

mjcarroll Jul 14, 2020

luccosta Jul 15, 2020

mjcarroll Jul 14, 2020

luccosta Jul 15, 2020

mjcarroll Jul 17, 2020

mjcarroll Jul 14, 2020

mjcarroll Jul 14, 2020

luccosta Jul 15, 2020 •

edited

Loading

mjcarroll Jul 14, 2020

luccosta Jul 15, 2020

mjcarroll Jul 17, 2020

luccosta commented Jul 15, 2020

luccosta left a comment

mjcarroll left a comment

	if (_v[0] > this->data[0])
	this->data[0] = _v[0];
	this->data[0] = std::max(_v[0], this->data[0]);

Vector3 to vector4 functions #132

Vector3 to vector4 functions #132

Conversation

luccosta commented Jul 13, 2020 • edited Loading

luccosta commented Jul 13, 2020

mjcarroll commented Jul 14, 2020

mjcarroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luccosta Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luccosta commented Jul 15, 2020

luccosta left a comment

Choose a reason for hiding this comment

mjcarroll left a comment

Choose a reason for hiding this comment

luccosta commented Jul 13, 2020 •

edited

Loading

luccosta Jul 15, 2020 •

edited

Loading