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

Add schema file name accessor to each DOM type C++ class #1376

Open
qingyouzhao opened this issue Feb 29, 2024 · 3 comments · May be fixed by #1393
Open

Add schema file name accessor to each DOM type C++ class #1376

qingyouzhao opened this issue Feb 29, 2024 · 3 comments · May be fixed by #1393
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@qingyouzhao
Copy link

Desired behavior

Getting the schema file for each SDF DOM type element should be possible.
This should make initializing an element for loading standalone DOM types easier.

Without this format. I am doing something like this in my code to initialize a DOM type element so that I can get type checking

// Schema file type
template <typename T>
struct SdfTypeToSchemaFile {
  static const std::string kSchemaFile;
};

template <>
const std::string SdfTypeToSchemaFile<sdf::Surface>::kSchemaFile = "surface.sdf";
// Other definitions with template specialization.

template <typename T>
sdf::ElementPtr init_element_for_type(){
  auto element = std::make_shared<::sdf::Element>();
  sdf::initFile(SdfTypeToSchemaFile<T>::kSchemaFile, element);
  return element;
}

Ideally, I hope to do something like this for simplicity.

template <typename T>
sdf::ElementPtr init_element_for_type(){
  auto element = std::make_shared<::sdf::Element>();
  sdf::initFile(T::SchemaFile(), element);
  return element;
}

Alternatives considered

Implementation suggestion

Implement a static function for each DOM type. For example for Surface:

// sdformat/include/sdf/Surface.hh
class Surface{
  // Omitted details
  public: static const std::string& SchemaFile();
};

// sdformat/src/Surface.cc

static const std::string& Surface::SchemaFile(){
  static const std::string* kSchemaFile = new std::string("surface.sdf");
  return *kSchemaFile;
}

Additional context

Currently the schema file is hard coded in it's T::ToElement() for example:

sdf::initFile("surface.sdf", elem);

@qingyouzhao qingyouzhao added the enhancement New feature or request label Feb 29, 2024
@azeey azeey added good first issue Good for newcomers help wanted Extra attention is needed labels Feb 29, 2024
@azeey azeey moved this from Inbox to To do in Core development Mar 11, 2024
@aagrawal05
Copy link

Hi, could you please assign this to me?
Just to confirm, what would be the list of all DOM elements which would need this feature?
Also would it be advisable to add a test for this in the default construction or is this unnecessary?

Thanks.

@qingyouzhao
Copy link
Author

Hi, could you please assign this to me? Just to confirm, what would be the list of all DOM elements which would need this feature? Also would it be advisable to add a test for this in the default construction or is this unnecessary?

Thanks.

The schemas are in sdformat/sdf/1.11 for the current version. The DOM elements corresponding are mostly located in sdformat/include. The names roughly matches each other regardless of the casing(snake_case for sdf and CamelCase for class).

@aagrawal05
Copy link

aagrawal05 commented Apr 3, 2024

Hi @qingyouzhao, I have implemented the required changes on my personal fork of the repository aagrawal05/sdformat on the schema_accessor branch.

Brief summary of the work I've done:

  1. I've created a script format_sdf.py which reads from the /sdf/1.11 folder and generates/updates the source for all the relevant classes in the src/ and include/ directories as per the suggested implementation.
  2. There were DOM elements navsat.sdf and forcetorque.sdf for which the script does not work and I had to manually edit the script because their naming convention did not match the camel-case conversion (they would need to benav_sat.sdf and force_torque.sdf) This may be undesirable from a code style perspective but may be a breaking change—this is a minor point but I just wanted to point it out.
  3. The code builds successfully, I have not thoroughly tested it but it does seem to work.
  4. I can add tests for the SchemaFile in all the DOM element tests but I thought it might be overkill because the names are hardcoded in the method anyways. I can add it easily with the script if needed.

I can delete the format_sdf.py from the commit as well, if it's no longer useful. Or I could place it in the sdf 1.11 folder similar to the embedSdf.py—not sure if it's relevant.

Could you please take a look and let me know if anything needs to be changed—I'd be happy to make any edits and open the pull request.

@aagrawal05 aagrawal05 linked a pull request Apr 5, 2024 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
Status: To do
Development

Successfully merging a pull request may close this issue.

3 participants