Skip to content

Commit

Permalink
Set sdf::Root to contain only one model/actor/light (#444)
Browse files Browse the repository at this point in the history 
* Set sdf::Root to contain only one model/actor/light
* Deprecated original functionality
* Switch to std::variant
* Revert previous API and logic, keeping new API
* Deprecate member variables
* Remove variable deprecation, revert root_dom integration test
* Update Migration.md

Signed-off-by: Stephen Brawner <brawner@gmail.com>
  • Loading branch information
@brawner
brawner authored Jan 9, 2021
1 parent b491e9b commit ed198a0
Show file tree
Hide file tree
Showing 20 changed files with 319 additions and 143 deletions.
22 changes: 22 additions & 0 deletions Migration.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ but with improved human-readability..
1. **sdf/Model.hh**:
+ std::pair<const Link *, std::string> CanonicalLinkAndRelativeName() const;

1. **sdf/Root.hh** sdf::Root elements can now only contain one of either Model,
Light or Actor since multiple items would conflict with overrides
specified in an <include> tag.
+ const sdf::Model \*Model();
+ const sdf::Light \*Light();
+ const sdf::Actor \*Actor();

### Modifications

1. **sdf/Model.hh**: the following methods now accept nested names relative to
Expand All @@ -36,6 +43,21 @@ but with improved human-readability..
+ bool JointNameExists(const std::string &) const
+ bool LinkNameExists(const std::string &) const

### Deprecations

1. **src/Root.hh**: The following methods have been deprecated in favor of the
new methods. For now the behavior is unchanged, but Root elements should
only contain one or none of Model/Light/Actor.
+ const sdf::Model \*ModelByIndex();
+ uint64_t ModelCount();
+ bool ModelNameExists(const std::string &\_name) const;
+ const sdf::Light \*LightByIndex();
+ uint64_t LightCount();
+ bool LightNameExists(const std::string &\_name) const;
+ const sdf::Actor \*ActorByIndex();
+ uint64_t ActorCount();
+ bool ActorNameExists(const std::string &\_name) const;

## SDFormat 9.x to 10.0

### Modifications
Expand Down
59 changes: 50 additions & 9 deletions include/sdf/Root.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,51 +127,92 @@ namespace sdf
/// \brief Get the number of models that are immediate (not nested) children
/// of this Root object.
/// \return Number of models contained in this Root object.
public: uint64_t ModelCount() const;
public: uint64_t ModelCount() const SDF_DEPRECATED(11.0);

/// \brief Get a model based on an index.
/// \param[in] _index Index of the model. The index should be in the
/// range [0..ModelCount()).
/// \return Pointer to the model. Nullptr if the index does not exist.
/// \sa uint64_t ModelCount() const
public: const Model *ModelByIndex(const uint64_t _index) const;
public: const sdf::Model *ModelByIndex(const uint64_t _index) const
SDF_DEPRECATED(11.0);

/// \brief Get whether a model name exists.
/// \param[in] _name Name of the model to check.
/// \return True if there exists a model with the given name.
public: bool ModelNameExists(const std::string &_name) const;
public: bool ModelNameExists(const std::string &_name) const
SDF_DEPRECATED(11.0);

/// \brief Get a pointer to the model object if it exists.
///
/// If there is more than one model, this will return the first element.
/// This method is preferred to ModelByIndex, as its behavior is
/// consistent with the planned future API. Having more than one Model, or
/// more than one of Model/Actor/Light is now considered deprecated and
/// should not be relied upon going forward.
///
/// \return A pointer to the model, nullptr if it doesn't exist
public: const sdf::Model *Model() const;

/// \brief Get the number of lights.
/// \return Number of lights contained in this Root object.
public: uint64_t LightCount() const;
public: uint64_t LightCount() const SDF_DEPRECATED(11.0);

/// \brief Get a light based on an index.
/// \param[in] _index Index of the light. The index should be in the
/// range [0..LightCount()).
/// \return Pointer to the light. Nullptr if the index does not exist.
/// \sa uint64_t LightCount() const
public: const Light *LightByIndex(const uint64_t _index) const;
public: const sdf::Light *LightByIndex(const uint64_t _index) const
SDF_DEPRECATED(11.0);

/// \brief Get whether a light name exists.
/// \param[in] _name Name of the light to check.
/// \return True if there exists a light with the given name.
public: bool LightNameExists(const std::string &_name) const;
public: bool LightNameExists(const std::string &_name) const
SDF_DEPRECATED(11.0);

/// \brief Get a pointer to the light object if it exists.
///
/// If there is more than one light, this will return the first element. If
/// there is already a model element, this will return null.
/// This method is preferred to LightByIndex, as its behavior is
/// consistent with the planned future API. Having more than one Light, or
/// more than one of Model/Actor/Light is now considered deprecated and
/// should not be relied upon going forward.
///
/// \return A pointer to the light, nullptr if it doesn't exist
public: const sdf::Light *Light() const;

/// \brief Get the number of actors.
/// \return Number of actors contained in this Root object.
public: uint64_t ActorCount() const;
public: uint64_t ActorCount() const SDF_DEPRECATED(11.0);

/// \brief Get an actor based on an index.
/// \param[in] _index Index of the actor. The actor should be in the
/// range [0..ActorCount()).
/// \return Pointer to the actor. Nullptr if the index does not exist.
/// \sa uint64_t ActorCount() const
public: const Actor *ActorByIndex(const uint64_t _index) const;
public: const sdf::Actor *ActorByIndex(const uint64_t _index) const
SDF_DEPRECATED(11.0);

/// \brief Get whether an actor name exists.
/// \param[in] _name Name of the actor to check.
/// \return True if there exists an actor with the given name.
public: bool ActorNameExists(const std::string &_name) const;
public: bool ActorNameExists(const std::string &_name) const
SDF_DEPRECATED(11.0);

/// \brief Get a pointer to the actor object if it exists.
///
/// If there is more than one actor, this will return the first element. If
/// there is already a model or light element, this will return null.
/// This method is preferred to ActorByIndex, as its behavior is
/// consistent with the planned future API. Having more than one Actor, or
/// more than one of Model/Actor/Light is now considered deprecated and
/// should not be relied upon going forward.
///
/// \return A pointer to the actor, nullptr if it doesn't exist
public: const sdf::Actor *Actor() const;

/// \brief Get a pointer to the SDF element that was generated during
/// load.
Expand Down
8 changes: 4 additions & 4 deletions src/FrameSemantics_TEST.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ TEST(FrameSemantics, buildFrameAttachedToGraph_Model)
EXPECT_TRUE(errors.empty()) << errors;

// Get the first model
const sdf::Model *model = root.ModelByIndex(0);
const sdf::Model *model = root.Model();

auto ownedGraph = std::make_shared<sdf::FrameAttachedToGraph>();
sdf::ScopedGraph<sdf::FrameAttachedToGraph> graph(ownedGraph);
Expand Down Expand Up @@ -217,7 +217,7 @@ TEST(FrameSemantics, buildPoseRelativeToGraph)
EXPECT_TRUE(root.Load(testFile).empty());

// Get the first model
const sdf::Model *model = root.ModelByIndex(0);
const sdf::Model *model = root.Model();

auto ownedGraph = std::make_shared<sdf::PoseRelativeToGraph>();
sdf::ScopedGraph<sdf::PoseRelativeToGraph> graph(ownedGraph);
Expand Down Expand Up @@ -313,7 +313,7 @@ TEST(NestedFrameSemantics, buildFrameAttachedToGraph_Model)
EXPECT_TRUE(errors.empty()) << errors;

// Get the first model
const sdf::Model *model = root.ModelByIndex(0);
const sdf::Model *model = root.Model();

auto ownedGraph = std::make_shared<sdf::FrameAttachedToGraph>();
sdf::ScopedGraph<sdf::FrameAttachedToGraph> graph(ownedGraph);
Expand Down Expand Up @@ -592,7 +592,7 @@ TEST(NestedFrameSemantics, ModelWithoutLinksWithNestedStaticModel)
auto errors = root.Load(testFile);
EXPECT_TRUE(errors.empty()) << errors;

const sdf::Model *model = root.ModelByIndex(0);
const sdf::Model *model = root.Model();
ASSERT_NE(nullptr, model);

auto ownedModelGraph = std::make_shared<sdf::FrameAttachedToGraph>();
Expand Down
90 changes: 83 additions & 7 deletions src/Root.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*
*/
#include <string>
#include <variant>
#include <vector>
#include <utility>

Expand All @@ -41,20 +42,32 @@ class sdf::RootPrivate
/// \brief The worlds specified under the root SDF element
public: std::vector<World> worlds;

/// \brief A model, light or actor under the root SDF element
/// This variant does not currently retain ownership, and instead points to
/// the first model/light/actor if they exist. When the vectors below are
/// removed this should be changed from a variant of pointers to a variant of
/// objects
public: std::variant<std::monostate, sdf::Model*, sdf::Light*, sdf::Actor*>
modelLightOrActor;

/// \brief The models specified under the root SDF element
public: std::vector<Model> models;
/// Deprecated: to be removed in libsdformat12
public: std::vector<sdf::Model> models;

/// \brief The lights specified under the root SDF element
public: std::vector<Light> lights;
/// Deprecated: to be removed in libsdformat12
public: std::vector<sdf::Light> lights;

/// \brief The actors specified under the root SDF element
public: std::vector<Actor> actors;
/// Deprecated: to be removed in libsdformat12
public: std::vector<sdf::Actor> actors;

/// \brief Frame Attached-To Graphs constructed when loading Worlds.
public: std::vector<sdf::ScopedGraph<FrameAttachedToGraph>>
worldFrameAttachedToGraphs;

/// \brief Frame Attached-To Graphs constructed when loading Models.
/// Deprecated: to be removed in libsdformat12 and converted to a single graph
public: std::vector<sdf::ScopedGraph<FrameAttachedToGraph>>
modelFrameAttachedToGraphs;

Expand All @@ -63,6 +76,7 @@ class sdf::RootPrivate
worldPoseRelativeToGraphs;

/// \brief Pose Relative-To Graphs constructed when loading Models.
/// Deprecated: to be removed in libsdformat12 and converted to a single graph
public: std::vector<sdf::ScopedGraph<PoseRelativeToGraph>>
modelPoseRelativeToGraphs;

Expand Down Expand Up @@ -254,9 +268,13 @@ Errors Root::Load(SDFPtr _sdf)
}

// Load all the models.
Errors modelLoadErrors = loadUniqueRepeated<Model>(
Errors modelLoadErrors = loadUniqueRepeated<sdf::Model>(
this->dataPtr->sdf, "model", this->dataPtr->models);
errors.insert(errors.end(), modelLoadErrors.begin(), modelLoadErrors.end());
if (!this->dataPtr->models.empty())
{
this->dataPtr->modelLightOrActor = &this->dataPtr->models.front();
}

// Build the graphs.
for (sdf::Model &model : this->dataPtr->models)
Expand All @@ -272,14 +290,42 @@ Errors Root::Load(SDFPtr _sdf)
}

// Load all the lights.
Errors lightLoadErrors = loadUniqueRepeated<Light>(this->dataPtr->sdf,
Errors lightLoadErrors = loadUniqueRepeated<sdf::Light>(this->dataPtr->sdf,
"light", this->dataPtr->lights);
errors.insert(errors.end(), lightLoadErrors.begin(), lightLoadErrors.end());
if (!this->dataPtr->lights.empty())
{
if (std::holds_alternative<std::monostate>(
this->dataPtr->modelLightOrActor))
{
this->dataPtr->modelLightOrActor = &this->dataPtr->lights.front();
}
else
{
sdfwarn << "Root object can only contain one of model, light or actor. "
<< "This behavior was deprecated in libsdformat11 and will soon "
<< "be removed";
}
}

// Load all the actors.
Errors actorLoadErrors = loadUniqueRepeated<Actor>(this->dataPtr->sdf,
Errors actorLoadErrors = loadUniqueRepeated<sdf::Actor>(this->dataPtr->sdf,
"actor", this->dataPtr->actors);
errors.insert(errors.end(), actorLoadErrors.begin(), actorLoadErrors.end());
if (!this->dataPtr->actors.empty())
{
if (std::holds_alternative<std::monostate>(
this->dataPtr->modelLightOrActor))
{
this->dataPtr->modelLightOrActor = &this->dataPtr->actors.front();
}
else
{
sdfwarn << "Root object can only contain one of model, light or actor. "
<< "This behavior was deprecated in libsdformat11 and will soon "
<< "be removed";
}
}

return errors;
}
Expand Down Expand Up @@ -322,7 +368,6 @@ bool Root::WorldNameExists(const std::string &_name) const
}
return false;
}

/////////////////////////////////////////////////
uint64_t Root::ModelCount() const
{
Expand Down Expand Up @@ -350,6 +395,17 @@ bool Root::ModelNameExists(const std::string &_name) const
return false;
}

/////////////////////////////////////////////////
const Model *Root::Model() const
{
if (std::holds_alternative<sdf::Model*>(this->dataPtr->modelLightOrActor))
{
return std::get<sdf::Model*>(this->dataPtr->modelLightOrActor);
}
return nullptr;
}


/////////////////////////////////////////////////
uint64_t Root::LightCount() const
{
Expand Down Expand Up @@ -377,6 +433,16 @@ bool Root::LightNameExists(const std::string &_name) const
return false;
}

/////////////////////////////////////////////////
const Light *Root::Light() const
{
if (std::holds_alternative<sdf::Light*>(this->dataPtr->modelLightOrActor))
{
return std::get<sdf::Light*>(this->dataPtr->modelLightOrActor);
}
return nullptr;
}

/////////////////////////////////////////////////
uint64_t Root::ActorCount() const
{
Expand Down Expand Up @@ -404,6 +470,16 @@ bool Root::ActorNameExists(const std::string &_name) const
return false;
}

/////////////////////////////////////////////////
const Actor *Root::Actor() const
{
if (std::holds_alternative<sdf::Actor*>(this->dataPtr->modelLightOrActor))
{
return std::get<sdf::Actor*>(this->dataPtr->modelLightOrActor);
}
return nullptr;
}

/////////////////////////////////////////////////
sdf::ElementPtr Root::Element() const
{
Expand Down
Loading

0 comments on commit ed198a0

Please sign in to comment.