[c++20] Rewrite Vector, Location2D, Matrix and LUDecomposition #782

TomSaw · 2021-11-15T13:00:01Z

This is an excerpt from #665.

modm::Vector<T, 2> is passed as template-parameter Resolution to modm::graphic::Buffer<..>
-> Thus the constructors require constexpr

By the chance, i may give some maintenance to all of these vector files.
~~Just modernization, no changes in logic~~ C++20 flavoured, more feature complete and 100% backwards compatible.

Key changes

Constexpr constructors everywhere
using <algorithm> everywhere (did i miss an oportunity? let me know!)
Applying std::round whenever float->integral conversion occurs
- GeometricTraits<T>::round() could be removed
[[depricated]] all Vector<>::convert() in fave of fresh conversion constructors .
Added some missing routines here and there, f.e IOStream operator<<
Improved readability
- using declaratives for local instances of Vector<T, N>
- (Re)assorted methods
- Migrated all method definitions into vector{N}.hpp

TODO

Re-add Vector<T, 2>::convert() and Location2D<U>::convert() for backwards-comp and depricate it
Treat deprecations
geometric_traits.hpp avr specialisation
Simplify the Vector tests

salkinium · 2021-11-15T14:28:58Z

Yes please. Constexpr constructors are particularly useful as they reduce to constinit .data section to be copied on startup instead of constructed by instructions.

TomSaw · 2021-11-15T19:22:12Z

Vector and Matrix use uint8_t as template and function parameter type for size metrics.
Shouldn't this be std::size_t or would it be an performance drawback when used as function parameter?

I think it may be desired to restrict these metrics to 0-255.

chris-durand · 2021-11-15T19:54:37Z

Vector and Matrix use uint8_t as template and function parameter type for size metrics.
Shouldn't this be std::size_t or would it be an performance drawback when used as function parameter?

That code was written for 8 bit AVR more than ten years ago. On those there is a penalty since arithmetic registers are 8 bit only but size_t is 16 bit. std::size_t is the right idiomatic choice. I guess std::size_t would be fine for operator[] on AVR. The compiler will inline it anyway.

TomSaw · 2021-11-15T19:57:00Z

Vector and Matrix use uint8_t as template and function parameter type for size metrics.
Shouldn't this be std::size_t or would it be an performance drawback when used as function parameter?

That code was written for 8 bit AVR more than ten years ago. On those there is a penalty since arithmetic registers are 8 bit only but size_t is 16 bit. std::size_t is the right idiomatic choice. I guess std::size_t would be fine for operator[] on AVR. The compiler will inline it anyway.

Great, so i'll take this to 2020!

TomSaw · 2021-11-16T23:58:24Z

Got some CI claims @windows only but no machine around..
Is there a smarter way than setting up a win-VM @salkinium ???

chris-durand · 2021-11-17T01:07:52Z

There are the same errors on Linux. You could be lucky and not need Windows to debug this.

TomSaw

Here's a complete rewrite, maybe a little too radical but feels like natural: modm::Vector is now an std::array with support for arithmetic operations.
I've benchmarked against the original code and found the exact same - in some cases even faster - execution times.

src/modm/math/geometry/vector.hpp

TomSaw · 2022-01-28T06:22:30Z

src/modm/math/geometry/vector.hpp

+
+		// no clue why, but RVO / Loop unrolling is not applied for this ...
+		// std::transform(this->begin(), this->end(), other.begin(), res.begin(), Func{});
+
+		// ... but this optimizes perfectly.
+		res.x() = BinFunc{}(this->x(), other.x());
+		if constexpr(N > 1) res.y() = BinFunc{}(this->y(), other.y());
+		if constexpr(N > 2) res.z() = BinFunc{}(this->z(), other.z());
+		if constexpr(N > 3) res.w() = BinFunc{}(this->w(), other.w());
+		if constexpr(N > 4) res.v() = BinFunc{}(this->v(), other.v());
+		if constexpr(N > 5) res.u() = BinFunc{}(this->u(), other.u());
+


For operators returning a new Vector, the <algorithm>-path did not optimize sufficiently (Line:174) - execution time has tripled. I've found a workaround (Line:177-182) but there's for sure a more elegant solution.

src/modm/math/geometry/vector.hpp

test/modm/math/geometry/vector3_test.cpp

chris-durand · 2022-01-31T18:39:18Z

@TomSaw I am quite busy with work right now, but will try to review tomorrow.

chris-durand

Thanks for all the effort you put into this. I got about halfway through your changes and will have a more detailed look when I find more time to do so.

src/modm/math/geometry/vector.hpp

src/modm/math/geometry/line_2d_impl.hpp

src/modm/math/geometry/line_segment_2d.hpp

src/modm/math/geometry/location_2d.hpp

chris-durand · 2022-02-01T20:23:44Z

src/modm/math/geometry/location_2d.hpp

+				position == other.position and
+				std::abs(orientation - other.orientation) < __FLT_EPSILON__


Can position be float? Should we handle that case somehow?

Not sure if __FLT_EPSILON__ is the right thing to do. For numbers between 1.0 and 2.0 it's the distance between adjacent floats. For numbers greater than 2.0 the comparison will do the same thing as ==.

src/modm/math/geometry/location_2d.hpp

src/modm/math/matrix.hpp

src/modm/math/geometry/vector.hpp

TomSaw · 2022-02-01T21:19:48Z

Oops... due to my vector deep dive, i've forgot to mention 🤦‍♂️:
Haven't yet taken the same care for these other geometry classes like i did for vector! This PRs title is kinda misleading.

If you find another minute, better take a second look on vector.hpp so that the new design is approved.
I'll take your review comments as inspirational boost into a last session with the other geometry candidates.

My pleasure. Also thanks for taking the time to review my code and support my practice! Win-win, i 💙 open-source.

TomSaw changed the title ~~constexpr for Vector<2> constructors~~ [math:vector] constexpr for Vector<2> constructors Nov 15, 2021

TomSaw changed the title ~~[math:vector] constexpr for Vector<2> constructors~~ [math:vector] constexpr constructors Nov 15, 2021

TomSaw mentioned this pull request Nov 15, 2021

[graphic] Rewrite graphics - Round 1 #665

Closed

20 tasks

TomSaw force-pushed the fix/constexpr-math-vector branch from b0fc65c to a97cf02 Compare November 15, 2021 13:56

TomSaw force-pushed the fix/constexpr-math-vector branch from a97cf02 to 2f525e4 Compare November 16, 2021 07:38

TomSaw changed the title ~~[math:vector] constexpr constructors~~ [math:vector] C++20 housekeeping Nov 16, 2021

TomSaw changed the title ~~[math:vector] C++20 housekeeping~~ [c++20] housekeeping math:vector Nov 16, 2021

TomSaw force-pushed the fix/constexpr-math-vector branch 9 times, most recently from 55ce405 to cab8d18 Compare November 16, 2021 21:21

TomSaw force-pushed the fix/constexpr-math-vector branch 8 times, most recently from 5760b8b to 11a5c8a Compare November 18, 2021 13:13

TomSaw force-pushed the fix/constexpr-math-vector branch 8 times, most recently from 7582629 to a6f7724 Compare January 27, 2022 21:01

TomSaw commented Jan 28, 2022

View reviewed changes

TomSaw force-pushed the fix/constexpr-math-vector branch 2 times, most recently from b57e8ed to 9af3c48 Compare January 28, 2022 20:32

TomSaw requested a review from chris-durand January 31, 2022 12:44

chris-durand reviewed Feb 1, 2022

View reviewed changes

TomSaw force-pushed the fix/constexpr-math-vector branch from ba50e27 to bf3e15e Compare February 18, 2022 09:25

TomSaw mentioned this pull request Feb 22, 2022

Add drawBitmap method to ColorGraphicsDisplay interface #831

Draft

Added tests for Vector basetype conversion

76794b2

TomSaw force-pushed the fix/constexpr-math-vector branch 8 times, most recently from 480cf7a to 291a456 Compare March 10, 2022 09:43

TomSaw force-pushed the fix/constexpr-math-vector branch from 291a456 to 9e2bb09 Compare March 15, 2022 21:49

TomSaw added 2 commits March 15, 2022 23:23

household/refactor modm::Vector and co

ea018d9

update /src and /examples, primary: x -> x(), y -> y(), z -> z()

30c76f3

TomSaw force-pushed the fix/constexpr-math-vector branch from 9e2bb09 to 30c76f3 Compare March 15, 2022 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[c++20] Rewrite Vector, Location2D, Matrix and LUDecomposition #782

[c++20] Rewrite Vector, Location2D, Matrix and LUDecomposition #782

TomSaw commented Nov 15, 2021 •

edited

Loading

salkinium commented Nov 15, 2021

TomSaw commented Nov 15, 2021 •

edited

Loading

chris-durand commented Nov 15, 2021 •

edited

Loading

TomSaw commented Nov 15, 2021

TomSaw commented Nov 16, 2021 •

edited

Loading

chris-durand commented Nov 17, 2021

TomSaw left a comment •

edited

Loading

TomSaw Jan 28, 2022 •

edited

Loading

chris-durand commented Jan 31, 2022

chris-durand left a comment

chris-durand Feb 1, 2022

TomSaw commented Feb 1, 2022 •

edited

Loading

		position == other.position and
		std::abs(orientation - other.orientation) < __FLT_EPSILON__

[c++20] Rewrite Vector, Location2D, Matrix and LUDecomposition #782

Are you sure you want to change the base?

[c++20] Rewrite Vector, Location2D, Matrix and LUDecomposition #782

Conversation

TomSaw commented Nov 15, 2021 • edited Loading

Key changes

TODO

salkinium commented Nov 15, 2021

TomSaw commented Nov 15, 2021 • edited Loading

chris-durand commented Nov 15, 2021 • edited Loading

TomSaw commented Nov 15, 2021

TomSaw commented Nov 16, 2021 • edited Loading

chris-durand commented Nov 17, 2021

TomSaw left a comment • edited Loading

Choose a reason for hiding this comment

TomSaw Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

chris-durand commented Jan 31, 2022

chris-durand left a comment

Choose a reason for hiding this comment

chris-durand Feb 1, 2022

Choose a reason for hiding this comment

TomSaw commented Feb 1, 2022 • edited Loading

TomSaw commented Nov 15, 2021 •

edited

Loading

TomSaw commented Nov 15, 2021 •

edited

Loading

chris-durand commented Nov 15, 2021 •

edited

Loading

TomSaw commented Nov 16, 2021 •

edited

Loading

TomSaw left a comment •

edited

Loading

TomSaw Jan 28, 2022 •

edited

Loading

TomSaw commented Feb 1, 2022 •

edited

Loading