[RST-1653] transaction stamps #37
Conversation
… into the Transaction itself. I couldn't help but to change the const accessors to return const objects while I was making changes.
… I may rethink this later; still deciding if the Graph's copy should be independent from the Transaction's copy.
| */ | ||
| virtual bool applyCallback(const std::set<ros::Time>& stamps, Transaction& transaction) = 0; | ||
| virtual bool applyCallback(Transaction& transaction) = 0; |
fuse_core/include/fuse_core/graph.h
Outdated
| @@ -58,7 +58,7 @@ namespace fuse_core | |||
| * be used in range-based for loops), an empty() method, and a front() method for directly accessing the first | |||
| * member. When dereferenced, an iterator returns a const Constraint&. | |||
| */ | |||
| using const_constraint_range = boost::any_range<const fuse_core::Constraint, boost::forward_traversal_tag>; | |||
| using const_constraint_range = boost::any_range<const Constraint, boost::forward_traversal_tag>; | |||
There was a problem hiding this comment.
I note that this header includes transaction.h, which also has this typedef, and in the same namespace. Note sure if you care. You probably don't want to assume anything about the typedefs in your includes.
There was a problem hiding this comment.
I noticed that as well. I definitely don't have a strong opinion, other than the current setup is inconsistent. Seems like I need to either:
(a) Move the typedefs in graph.h inside the Graph class. This is what was done in the Transaction object, and it prevents any sort of name collision.
(b) Move all of these range definitions into a common file somewhere and include that whenever I need them.
Thoughts? Preferences?
There was a problem hiding this comment.
I like (a) better. Presumably, (b) would require that the types.h file (or whatever) includes constraint.h, transaction.h, etc. If that's true, then might we not get into weird circular dependencies where including the types.h file?
fuse_core/src/timestamp_manager.cpp
Outdated
| Transaction motion_model_transaction; | ||
| auto first_stamp = *stamps.begin(); | ||
| auto last_stamp = *stamps.begin(); | ||
| for (const auto& stamp : stamps) |
There was a problem hiding this comment.
Won't this just end up making last_stamp == stamps.back()? Or is there something about this iteration that might terminate before we reach stamps.end()? Because otherwise you could do
auto last_stamp = stamps.back();
std::for_each(stamps.begin(), stamps.end(), motion_model_transaction.addInvolvedStamp);
Not sure if a bind is required for the last param. You could also add an overload to the transaction to add a range of involved stamps. But again, this assumes that there isn't some logic that I'm missing with last_stamp.
There was a problem hiding this comment.
A couple of things:
(a) For reasons that I don't fully buy into, std::set (and others) don't have a back() method. They do have an rbegin() method.
(b) For reasons that seemed important at the time, I am obfuscating the underlying container inside the Transaction object. The public API provides a forward-only range. I don't believe the forward-only range has an rbegin()...because then it would be a bidirectional range.
That explains why the code does what it does.
Whether it should be that way is a different question. I have the ability to return a bidirectional range. I have the ability to just return the set. Philosophically I like the interface-implementation separation the current setup provides. But I'm never going to actually use that ability. It is far more important for the Graph, where I am just defining the interface, and the implementation is left up to derived classes.
Discuss.
There was a problem hiding this comment.
Well, my initial guess was that I was missing something, and indeed I was. So I'd say leave it as-is, because I agree with your reasoning. :)
There was a problem hiding this comment.
Yeah. I can't find a solution I like any better. I can do:
std::for_each(
stamps.begin(),
stamps.end(),
std::bind(&Transaction::addInvolvedStamp, &motion_model_transaction, std::placeholders::_1));
..but that doesn't look any cleaner. And I'd still need to find the last stamp in a separate pass. Strangely I can't find an existing algorithm that does that.
There was a problem hiding this comment.
Yeah, I'd say keep it your way.
| return stamp_range(boost::make_transform_iterator(motion_model_history_.begin(), extractStamp), | ||
| boost::make_transform_iterator(motion_model_history_.end(), extractStamp)); | ||
| } | ||
| auto extract_stamp = +[](const MotionModelHistory::value_type& element) -> const ros::Time& |
There was a problem hiding this comment.
The fancy + syntax might be worth a comment.
I'm sorry....
This seemed like a simple change...
Then it rippled to a lot more places than I expected.
Might make sense to walk through the changes on a per-commit basis.
Anyway, whenever a sensor/motion model publishes a Transaction, it also sends a set of "involved timestamps" with it. Since the two always go together, I thought "Why not put them together?" It has the added bonus of moving the timestamp container around via pointer instead of via copy...because the Transaction is moved around via pointer.
There will be additional ripples to
fuse_rlandfuse_locuslater.