Skip to content
Open
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
32 changes: 32 additions & 0 deletions lib/sdf/erb.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# frozen_string_literal: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please split code in modules only when you see that things get too big, or that you need the functionality in multiple places. Not when you think it/you will.

The functionality of erb.rb should really be straight into ERBLoader#parse_sdf_document

@Rezenders Rezenders Aug 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, I will include it in the ERBLoader class.


require "erb"

module SDF
# Module for handling ERB files
module ERB
module_function

Comment on lines +8 to +9

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Avoid this. It's very easy to miss it when the module gets bigger.

There are two better patterns:

  1. define each method with self., e.g. def self.parse_erb_as_str
  2. define the module normally and do an extend self at the very end

The latter is also hard to miss, but the module in itself behaves normally (one can include it elsewhere and it works). I prefer (1) when the purpose of the module is really to be a namespace.

# Parses an ERB string and returns the raw rendered string
#
# @param [String] erb_content ERB template file content as string
# @param [Hash] erb_args the configuration arguments to evaluate
# @return [String] the raw rendered XML string representing the model
def parse_erb_as_str(erb_content, **erb_args)
erb_engine = ::ERB.new(erb_content, trim_mode: "-")

# Render the ERB template with the passed hash arguments
erb_engine.result_with_hash(erb_args)
end

# Renders an ERB template and returns it as a REXML::Document
#
# @return [REXML::Document] the rendered sdf model
def render_erb_sdf_model(path, **erb_args)
erb_content = File.read(path)
solved_erb_as_sdf_str = parse_erb_as_str(erb_content, **erb_args)

REXML::Document.new(solved_erb_as_sdf_str)
end
end
end
18 changes: 18 additions & 0 deletions lib/sdf/erb_loader.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

require_relative "erb"
require_relative "sdf_loader"

module SDF
# class to load SDF and ERB templated SDF files
class ERBLoader < Loader

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm missing the purpose or advantage of having separate loader and ERBLoader classes. This stuff is so simple, why not a single class ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If I remember correctly, @jhonasiv requested me to do it so we could enforce that when the SDF::Loader is configured only .sdf files are loaded

def initialize(erb_args: {})
super()
@erb_args = erb_args
end

def parse_sdf_document(sdf_file)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you should make a difference between erb and non-erb files, that is parse ERB only when the extension is .erb

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I told him to do it like this. If you mean for having an explicit error when a non-erb file is given to an ERBLoader, I feel this is overkill and it would painful in the Robot level to constantly juggle between loaders when the model file changes (I dont think you mean this, just getting it out there).

In the case you want to split the functionality between parse_sdf_document, or do a plain load directly as its done nowadays when the file does not have a .erb, my concern would be the flakiness of someone defining a model.sdf that IS an file with ERB variables on it without realizing, and then the syntax error when interpreting the SDF would be probably very noisy.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree with @jhonasiv on this one, but in this case I would simply remove the base SDF::Loader class and keep only the ERBLoader.

In case we enforce the files to end with .erb then I suggest we keep both loaders and make them only handle their specific file extension

But anyway, I don't have a strong opinion on this, so I would happily go with any

SDF::ERB.render_erb_sdf_model(sdf_file, **@erb_args)
end
end
end
21 changes: 21 additions & 0 deletions lib/sdf/exceptions.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,24 @@
module SDF
class InternalError < RuntimeError; end

module XML
# Exception raised when trying to load a model URI, but the model does
# not contain a SDF entry for the required SDF version
class UnavailableSDFVersionInModel < ArgumentError; end
# Exception raised when trying to load a file that is not a SDF file
class NotSDF < ArgumentError; end
# Exception raised when trying to load a malformed XML file
class InvalidXML < ArgumentError; end

# Exception raised when trying to resolve a model that cannot be found
# in {model_path}
class NoSuchModel < ArgumentError
attr_reader :model_name

def initialize(model_name)
super
@model_name = model_name
end
end
end
end
10 changes: 5 additions & 5 deletions lib/sdf/root.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ def initialize(xml, metadata = {})
# @raise [XML::NotSDF] if the file is not a SDF file
# @raise [XML::InvalidXML] if the file is not a valid XML file
# @return [Root]
def self.load(sdf_file, expected_sdf_version = nil, flatten: true)
def self.load(sdf_file, expected_sdf_version = nil, flatten: true, loader: SDF::Loader.new)
if sdf_file =~ %r{^model://(.*)}
load_from_model_name(::Regexp.last_match(1), expected_sdf_version,
flatten: flatten)
flatten: flatten, loader: loader)
else
xml, metadata = XML.load_sdf(sdf_file, flatten: flatten, metadata: true)
xml, metadata = XML.load_sdf(sdf_file, flatten: flatten, metadata: true, loader: loader)
new(xml.root, metadata)
end
end
Expand All @@ -48,9 +48,9 @@ def self.load(sdf_file, expected_sdf_version = nil, flatten: true)
# (as version * 100, i.e. version 1.5 is represented by 150). Leave to
# nil to always read the latest.
# @return [Root]
def self.load_from_model_name(model_name, sdf_version = nil, flatten: true)
def self.load_from_model_name(model_name, sdf_version = nil, flatten: true, loader: SDF::Loader.new)
xml, metadata = XML.model_from_name(model_name, sdf_version,
flatten: flatten, metadata: true)
flatten: flatten, metadata: true, loader: loader)
new(xml.root, metadata)
end

Expand Down
64 changes: 64 additions & 0 deletions lib/sdf/sdf_loader.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# frozen_string_literal: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SDF::Loader lives in sdf/loader.rb not sdf/sdf_loader.rb


require_relative "erb"
require_relative "exceptions"

module SDF
# class to load SDF and ERB templated SDF files
class Loader
# Open a SDF file SDF file and returns its XML representation.
#
# @param [String] sdf_file the path to the SDF file
# @raise [Errno::ENOENT] if the files does not exist
# @raise [NotSDF] if the file is not a SDF file
# @raise [InvalidXML] if the file is not a valid XML file
# @return [REXML::Element] sdf_file's content as a REXML::Element instance
def load_sdf_raw(sdf_file)
find_or_raise_file_not_found(sdf_file)

sdf = begin
parse_sdf_document(sdf_file)
rescue REXML::ParseException => e
unless e.message.include?("No root")
raise SDF::XML::InvalidXML,
"Cannot load #{sdf_file}: #{e.message}"
end

REXML::Document.new

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know that it was in the original code, but ...

I think the value of having an error message that says "it loads as XML but it has no root" rather than "this file has no root" does not warrant the complexity of having a codepath dedicated for it. Please simplify it.

end
validate_sdf_root(sdf, sdf_file)

sdf
end

private

def find_or_raise_file_not_found(sdf_file)
return if File.exist?(sdf_file)

file_name = File.basename(sdf_file)
dir_path = File.dirname(sdf_file)
raise Errno::ENOENT,
"Cannot find '#{file_name}' in '#{dir_path}'." \
"You probably want to update the GAZEBO_MODEL_PATH " \
"environment variable, or set SDF.model_path explicitly."
Comment on lines +41 to +44

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This class knows nothing about GAZEBO_MODEL_PATH or SDF.model_path. It is given an already resolved path as argument.

You should assume that the argument exists and let ENOENT propagate. The levels that do resolution based on GAZEBO_MODEL_PATH should error out if a model does not exist.

end

def parse_sdf_document(sdf_file)
File.open(sdf_file) do |io|
REXML::Document.new(io)
end
end

def validate_sdf_root(sdf, sdf_file)
unless sdf.root
raise SDF::XML::NotSDF,
"#{sdf_file} can be parsed as an XML file, but it " \
"does not have a root"
end
return if %w[sdf gazebo].include?(sdf.root.name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

gazebo as a root ? Is that valid SDF ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't think so: https://sdformat.org/spec/1.12/sdf/
I included it because it was in the original code and I didn't know whether there was a reason for it.
I will remove it then


raise SDF::XML::NotSDF, "#{sdf_file} is not a SDF file"
end
end
end
120 changes: 55 additions & 65 deletions lib/sdf/xml.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
require "rexml/document"
require_relative "exceptions"
require_relative "sdf_loader"
require_relative "erb_loader"
Comment on lines +2 to +4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We do not use require_relative.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Honest question, why is that? I thought require_relative was less error prune


module SDF
module XML
Expand All @@ -7,24 +10,6 @@ module XML
# (as version * 100, i.e. version 1.5 is represented by 150). Leave to
# nil to always read the latest.

# Exception raised when trying to load a model URI, but the model does
# not contain a SDF entry for the required SDF version
class UnavailableSDFVersionInModel < ArgumentError; end
# Exception raised when trying to load a file that is not a SDF file
class NotSDF < ArgumentError; end
# Exception raised when trying to load a malformed XML file
class InvalidXML < ArgumentError; end

# Exception raised when trying to resolve a model that cannot be found
# in {model_path}
class NoSuchModel < ArgumentError
attr_reader :model_name

def initialize(model_name)
@model_name = model_name
end
end

# The search path for models
#
# It defaults to GAZEBO_MODEL_PATH
Expand Down Expand Up @@ -83,9 +68,9 @@ def self.clear_cache
# SDF file for the required SDF version
#
# @return [REXML::Element]
def self.load_gazebo_model(dir, sdf_version = nil, metadata: false, flatten: true)
def self.load_gazebo_model(dir, sdf_version = nil, metadata: false, flatten: true, loader: SDF::Loader.new)
load_sdf(model_path_of(dir, sdf_version), metadata: metadata,
flatten: flatten)
flatten: flatten, loader: loader)
end

# Find model string into model.config path
Expand Down Expand Up @@ -128,7 +113,7 @@ def self.model_path_of(dir, sdf_version = nil)
#
# @!macro sdf_version
# @return [Hash<String,REXML::Element>]
def self.gazebo_models(sdf_version = nil)
def self.gazebo_models(sdf_version = nil, loader: SDF::ERBLoader.new)
@gazebo_models[sdf_version] ||= {}
@model_path.each do |p|
Dir.glob(File.join(p, "*")) do |subdir|
Expand All @@ -140,7 +125,7 @@ def self.gazebo_models(sdf_version = nil)
begin
sdf_file_path = model_path_of(subdir, sdf_version)
sdf, metadata = load_sdf(sdf_file_path, metadata: true,
flatten: false)
flatten: false, loader: loader)
@gazebo_models[sdf_version][File.basename(subdir)] =
ModelCacheEntry.new(sdf_file_path, sdf, metadata)
rescue UnavailableSDFVersionInModel
Expand All @@ -165,7 +150,7 @@ def self.gazebo_models(sdf_version = nil)
# @raise (see model_path_of)
# @raise [NoSuchModel] if the provided model name does not resolve to a
# model in {model_path}
# @return [REXML::Element]
# @return [String] the path to the SDF file for the model
def self.model_path_from_name(model_name, model_path: @model_path, sdf_version: nil)
@gazebo_models[sdf_version] ||= {}
cache = (@gazebo_models[sdf_version][model_name] ||= ModelCacheEntry.new)
Expand Down Expand Up @@ -193,12 +178,12 @@ def self.model_path_from_name(model_name, model_path: @model_path, sdf_version:
# model in {model_path}
# @return [REXML::Element]
def self.model_from_name(
model_name, sdf_version = nil, metadata: false, flatten: true
model_name, sdf_version = nil, metadata: false, flatten: true, loader: SDF::Loader.new
)
path = model_path_from_name(model_name, sdf_version: sdf_version)
cache = @gazebo_models[sdf_version][model_name]
unless cache.xml
cache.xml, cache.metadata = load_sdf(path, metadata: true, flatten: false)
cache.xml, cache.metadata = load_sdf(path, metadata: true, flatten: false, loader: loader)
end
xml = cache.xml
if flatten
Expand All @@ -213,6 +198,23 @@ def self.model_from_name(
end
end

# Resolves relative paths and model:// URIs in the XML tree in-place
#
# This method traverses the XML tree starting from the given node, and
# expands any relative paths or `model://` URIs inside `<uri>` tags to
# absolute paths on the local filesystem.
#
# It skips `<include>` tags because those are resolved separately during
# {.add_include_tags}.
#
# @example Replaces a model:// mesh path:
# # Before: <uri>model://robot_model/hull.dae</uri>
# # After: <uri>/path/to/workspace/robot_models/models/sdf/robot_model/hull.dae</uri>
#
# @param [REXML::Element] node the XML element to traverse
# @!macro sdf_version
# @param [String] base_path the base directory path used to resolve relative paths
# @return [void]
def self.resolve_relative_uris(node, sdf_version, base_path)
nodes = [node]
until nodes.empty?
Expand Down Expand Up @@ -264,16 +266,34 @@ def self.deep_copy_xml(node)
# This method modifies the XML tree by replacing the include tags found
# as direct children of the provided element by the included content.
#
# @example
# # Before calling add_include_tags:
# # <world name="my_world">
# # <include>
# # <uri>model://my_sensor</uri>
# # <name>custom_sensor</name>
# # <pose>1 0 0 0 0 0</pose>
# # </include>
# # </world>
# #
# # After calling add_include_tags:
# # <world name="my_world">
# # <model name="custom_sensor">
# # <pose>1 0 0 0 0 0</pose>
# # <link name="sensor_link">...</link>
# # </model>
# # </world>
#
# @param [REXML::Element] elem element to find include tags
# @!macro sdf_version
# @return [void]
def self.add_include_tags(elem, sdf_version, base_path)
def self.add_include_tags(elem, sdf_version, base_path, loader: SDF::Loader.new)
includes = {}

replacements = []
elem.elements.each do |inc|
if inc.name == "world" || inc.name == "model" # model-within-model
added_includes = add_include_tags(inc, sdf_version, base_path)
added_includes = add_include_tags(inc, sdf_version, base_path, loader: loader)
includes.merge! added_includes do |_, old, new|
old + new
end
Expand Down Expand Up @@ -309,11 +329,11 @@ def self.add_include_tags(elem, sdf_version, base_path)

included_sdf, included_metadata =
model_from_name(model_name, sdf_version, metadata: true,
flatten: false)
flatten: false, loader: loader)
elsif File.directory?(uri_path = File.expand_path(uri, base_path))
included_sdf, included_metadata =
load_gazebo_model(uri_path, sdf_version, metadata: true,
flatten: false)
flatten: false, loader: loader)
else
raise ArgumentError,
"URI #{uri} is neither a model:// URI nor an existing directory"
Expand Down Expand Up @@ -362,39 +382,6 @@ def self.add_include_tags(elem, sdf_version, base_path)
includes
end

# Open a SDF file and returns the XML representation
#
# Unlike {.load_sdf}, this really only loads the XML information, not
# resolving the include tags.
#
# @param [String] sdf_file the path to the SDF file
# @raise [Errno::ENOENT] if the files does not exist
# @raise [NotSDF] if the file is not a SDF file
# @raise [InvalidXML] if the file is not a valid XML file
# @return [REXML::Element]
def self.load_sdf_raw(sdf_file)
sdf = File.open(sdf_file) do |io|
REXML::Document.new(io)
rescue REXML::ParseException => e
unless e.message.match?(/No root/)
raise InvalidXML, "cannot load #{sdf_file}: #{e.message}"
end

REXML::Document.new
end

unless sdf.root
raise NotSDF,
"#{sdf_file} can be parsed as an XML file, but it does not have a root"
end

if sdf.root.name != "sdf" && sdf.root.name != "gazebo"
raise NotSDF, "#{sdf_file} is not a SDF file"
end

sdf
end

# Get sdf_version
#
# @param [REXML::Element] sdf element
Expand Down Expand Up @@ -441,19 +428,22 @@ def self.sdf_version_of(sdf)
# @param [Boolean] metadata whether the method should return a metadata hash
# about the various inclusions that have been performed. See above for
# the hash format
# @param [#load_sdf_raw] loader object that acts as a loader.Takes a file path as input
# and returns a REXML::Element with its content. Must respond to
# `load_sdf_raw(path: String) -> REXML::Element`
# @return [REXML::Element,(REXML::Element,Hash)] either the XML tree by itself
# if `metadata` is false, or the pair of the tree and the metadata hash
# otherwise.
# @raise [Errno::ENOENT] if the files does not exist
# @raise [NotSDF] if the file is not a SDF file
# @raise [InvalidXML] if the file is not a valid XML file
# @return [REXML::Element]
def self.load_sdf(sdf_file, flatten: true, metadata: false)
sdf = load_sdf_raw(sdf_file)
def self.load_sdf(sdf_file, flatten: true, metadata: false, loader: SDF::Loader.new)
sdf = loader.load_sdf_raw(sdf_file)
sdf_version = sdf_version_of(sdf)

sdf_metadata = Hash["includes" => {}, "path" => sdf_file]
includes = add_include_tags(sdf.root, sdf_version, File.dirname(sdf_file))
includes = add_include_tags(sdf.root, sdf_version, File.dirname(sdf_file), loader: loader)
sdf_metadata["includes"].merge!(includes) do |_, old, new|
old + new
end
Expand Down
Loading