-
Notifications
You must be signed in to change notification settings - Fork 0
New Concept structure #21
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
Conversation
| @@ -0,0 +1,76 @@ | |||
| #ifndef VORTEX_UTILS__ACCESSORS_HPP_ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of creating functions to access publicly exposed members? Further, just curious, what are the specific usecases of all these generic functions other than possibly needing them in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The accessors are there so that arbitraty "pose" types can satisy the concept requirements. The generic functions like the ones in "views.hpp" can in the future, instead of having to define these functions for each new type, we just need to define the accessor functions and the generic functions will work.
include/vortex/utils/types.hpp
Outdated
| /** | ||
| * @brief Eigen-based pose, for interfaces / measurements | ||
| */ | ||
| struct PoseQuatEigen { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why keep this and eta? They contain the same data just in different forms. The codebase needs to be refactored anyway to apply the other freefunctions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this does not need to be defined as is with eigen types. I am considering just creating a POD Pose struct, because Eta is not really a familiar term for perception
|
I reverted the Eta types back to their original structure. Also created a Pose struct that the perception boys can use. I don't know exactly how "domain" specific the eta structs are, if that is the case then having them more Object-oriented might make things more explicit and intent more clear. For general Pose structs giving them access the concept capabilities and the views functions should be sufficient for now. This setup IMO can achieve a good middle ground |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 98.11% 98.14% +0.03%
==========================================
Files 5 6 +1
Lines 425 432 +7
Branches 97 84 -13
==========================================
+ Hits 417 424 +7
Misses 4 4
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Experimented with new structure for utils.
Adds more boilerplate but should be more scalable.
By simply defining some acessors for your type you can gain access to functions guarded by concepts.
Why bother defining accessors like these:
Just to get access to a simple function like this:
Well this design ensures better scalability when more types(structs) and functions are added.
It allows us to create general function like this, without increasing complexity too much:
A downside is that we lose some explicitness. Most of the
etafunctions inops.hppcould probably have been converted to more general purpose functions using the concepts, but at the loss of intent.Thoughts?