Skip to content
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

Simulator agnostic agents #481

Merged
merged 1 commit into from
Jul 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/delphyne/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ target_link_libraries(public_headers
# Files
install(
FILES
agent_base.h
macros.h
types.h
${PROJECT_BINARY_DIR}/include/${PROJECT_NAME}/version.h
Expand All @@ -50,3 +49,4 @@ install(TARGETS public_headers EXPORT ${PROJECT_NAME}-targets)
##############################################################################

add_subdirectory(maliput)
add_subdirectory(mi6)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it :).

97 changes: 0 additions & 97 deletions include/delphyne/agent_base.h

This file was deleted.

26 changes: 13 additions & 13 deletions include/delphyne/macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,18 @@
/// Used to validate that an argument passed into a function or method is true;
/// if not, an exception of type exctype is thrown.

#define DELPHYNE_VALIDATE(pred, exctype, message) \
do { \
if (!(pred)) { \
std::string fullname = std::string(__FILE__); \
size_t found = fullname.find_last_of("/"); \
std::string fname = fullname; \
if (found != std::string::npos) { \
fname = fullname.substr(found+1); \
} \
std::string errmsg(fname); \
#define DELPHYNE_VALIDATE(pred, exctype, message) \
do { \
if (!(pred)) { \
std::string fullname = std::string(__FILE__); \
size_t found = fullname.find_last_of("/"); \
std::string fname = fullname; \
if (found != std::string::npos) { \
fname = fullname.substr(found + 1); \
} \
std::string errmsg(fname); \
errmsg.append(":").append(__func__).append(":").append(STR(__LINE__)); \
errmsg.append(": ").append(message); \
throw exctype(errmsg); \
} \
errmsg.append(": ").append(message); \
throw exctype(errmsg); \
} \
} while (0)
31 changes: 31 additions & 0 deletions include/delphyne/mi6/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
###############################################################################
# Install
###############################################################################

add_library(mi6 INTERFACE)

target_include_directories(
mi6
INTERFACE
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include/${PROJECT_NAME}>
)

target_link_libraries(mi6
INTERFACE
public_headers
drake::drake
)

# Install files
install(
FILES
agent_base.h
agent_diagram_builder.h
diagram_bundle.h
DESTINATION
${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}${PROJECT_MAJOR_VERSION}/${PROJECT_NAME}/mi6
)

# Install the exported target
install(TARGETS mi6 EXPORT ${PROJECT_NAME}-targets)
116 changes: 116 additions & 0 deletions include/delphyne/mi6/agent_base.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
// Copyright 2017 Toyota Research Institute

#pragma once

/*****************************************************************************
** Includes
*****************************************************************************/

#include <memory>
#include <set>
#include <string>

#include <drake/common/eigen_types.h>
#include <drake/geometry/geometry_ids.h>

#include "delphyne/mi6/agent_diagram_builder.h"
#include "delphyne/mi6/diagram_bundle.h"
#include "delphyne/types.h"

/*****************************************************************************
** Namespaces
*****************************************************************************/

namespace delphyne {

/*****************************************************************************
** Interfaces
*****************************************************************************/

/// @brief The parent of all agents in delphyne!
///
/// This is the abstract class that all delphyne agents must inherit from.
/// Concrete implementations are required to implement the BuildDiagram()
/// method which packages this agent's sensor-planner-control systems
/// into a single diagram for encapsulation into the simulator's main
/// diagram.
///
/// ::DiagramBuilder and ::DiagramBundle can be used to assist in the
/// diagram construction. In particular, refer to the documentation of
/// ::DiagramBuilder for information about the agent diagram structure
/// and convenience methods.
///
/// @tparam One of double, delphyne::AutoDiff or delphyne::Symbolic.
template <typename T>
class AgentBase {
public:
/// Type for packaging diagram and indices together in a bundle.
using DiagramBundle = delphyne::DiagramBundle<T>;
/// Specific builder for this agent. Use inside BuildDiagram().
using DiagramBuilder = AgentDiagramBuilder<T>;

/// @brief Constructor initialising common agent parameters.
///
/// @param name[in]: Convenient descriptive name for the agent
/// (must be unique in any given simulation).
explicit AgentBase(const std::string& name) : name_(name) {}
virtual ~AgentBase() = default;

/// @brief The agent diagram builder
///
/// This diagram encloses the systems needed for the agent to run and
/// will ultimately be encapsulated in the simulator's diagram.
///
/// @return DiagramBundle : The diagram along with the relevant input
/// and output indices for an agent diagram that allow it to be
/// wired up with systems in the simulator diagram.
virtual std::unique_ptr<DiagramBundle> BuildDiagram() const = 0;

/// @brief Name accessor
const std::string& name() const { return name_; }

/// @brief Accessor to the geometry ids
const std::set<drake::geometry::GeometryId>& geometry_ids() const {
return geometry_ids_;
}

/// @brief Mutable accessor to the geometry ids
std::set<drake::geometry::GeometryId>& mutable_geometry_ids() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stonier it is a standard practice in Drake to use pointers instead of mutable references.

Copy link
Collaborator Author

@stonier stonier Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to kick back on that. I really, really, really don't want to be coerced into thinking I must check for null every time I call this method. Perhaps I'm missing some logic for why they do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I can't do mind control :). But if you're worried about pointer integrity, I'd perform the necessary checks within the accessor if it applies (just like you'd do before returning a reference) and then all user code needs not worry about it (as long as the documentation clearly states that you don't have to and a nullptr won't ever happen).

Having said that, I've seen mutable references being returned in some parts of Drake. So, in any case, I would advocate for consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not in the style guide and I don't think we should introduce an arbitrary relaxation of the function contract just to follow some pattern that may have merely become habit without good reason elsewhere. Happy to be swayed if there is a good reason.

Having said that, not so happy with using mutable values to set a variable here. Would rather make use of a setter since this is not an expensive, high frequency operation, but it wasn't written that way originally and would likely be revisited when working out how to incorporate geometries more appropriately in agents.

return geometry_ids_;
}

/// @brief Accessor for the initial world pose of the agent.
const drake::Isometry3<double>& initial_world_pose() const {
return initial_world_pose_;
}

/// Checks whether this agent is the source for the given
/// @p geometry_id (i.e. has registered the geometry associated
/// with that id) or not.
virtual bool is_source_of(
const drake::geometry::GeometryId& geometry_id) const {
return (geometry_ids_.count(geometry_id) != 0);
}

protected:
std::string name_;
// TODO(daniel.stonier) stop using this, make use of an
// initial value on the pose output (used by geometry settings for
// the collision subsystem)
drake::Isometry3<double> initial_world_pose_;

private:
// TODO(daniel.stonier) dubious whether we should have
// simulator specific machinery here
std::set<drake::geometry::GeometryId> geometry_ids_{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stonier FYI when I first added this, the rationale was to keep collision geometries an agent implementation detail.

};

/*****************************************************************************
** Typedefs
*****************************************************************************/

using Agent = AgentBase<double>;
using AutoDiffAgent = AgentBase<AutoDiff>;
using SymbolicAgent = AgentBase<Symbolic>;

} // namespace delphyne
Loading