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

Export package instead of URDF #187

Merged

Conversation

audrow
Copy link
Contributor

@audrow audrow commented Nov 10, 2023

This PR adds the business logic from #186 into rmf_site_editor.

Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Very nice, just a few minor changes here and there but I think this is pretty much good to go

let context = tera::Context::from_serialize(context)?;
let mut tera = Tera::default();
for template in templates.iter() {
let content = std::fs::read_to_string(&template.path)?;
Copy link
Member

Choose a reason for hiding this comment

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

This seems to fail when users do cargo run from a folder that is not the root of the workspace, since the path is in the form of rmf_site_editor/[...], which is only valid from the root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect that this solves this. Any ideas?

rmf_site_editor/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 168 to 187
if let AssetSource::Package(_) = asset_source {
let path = String::from(asset_source);
Ok((*urdf_rs::utils::expand_package_path(&path, None)).to_owned())
} else if let AssetSource::Remote(asset_name) = asset_source {
let mut asset_path = cache_path();
asset_path.push(PathBuf::from(&asset_name));
Ok(asset_path
.to_str()
.ok_or(IoError::new(
IoErrorKind::InvalidInput,
"Unable to convert asset_path to str",
))?
.to_string())
} else if let AssetSource::Local(path) = asset_source {
Ok(path.clone())
} else {
Err(IoError::new(
IoErrorKind::Unsupported,
"Not a supported asset type for exporting a workcell to a package",
))?
Copy link
Member

Choose a reason for hiding this comment

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

This would be a bit more idiomatic written as a match statement rather than an if let chain, the unimplemented branch should probably also have all the variants written explicitly so we can make it a compile error if in the future a variant is added, i.e.:

match asset_source {
    AssetSource::Package(_) => [...],
    AssetSource::Remote(asset_name => [...],
    AssetSource::Local(path) => [...],
    AssetSource::Search(_) | AssetSource::OSMTile(_) | [...] => return the error
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in ee15eac.

rmf_site_editor/src/workcell/save.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/workcell/save.rs Outdated Show resolved Hide resolved
@luca-della-vedova
Copy link
Member

luca-della-vedova commented Nov 17, 2023

Another note, I'm a bit worried about the GPG signing since this repo requires all commits to be verified, can you check if there is any issue with your GPG key then force-push? It shows Unverified next to each of your commits.

Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
@audrow
Copy link
Contributor Author

audrow commented Nov 21, 2023

Another note, I'm a bit worried about the GPG signing since this repo requires all commits to be verified, can you check if there is any issue with your GPG key then force-push? It shows Unverified next to each of your commits.

All verified now!

@luca-della-vedova luca-della-vedova merged commit 4b0b3b6 into open-rmf:luca/urdf_import Nov 21, 2023
5 checks passed
@luca-della-vedova
Copy link
Member

I can't push changes to the branch so I'll just merge this in and push directly to the feature branch. For reference this is the commit that fixed the file issue (using the include_bytes! macro) as well as a new issue that popped up during refactor where package:// paths were not being generated correctly.

@audrow audrow deleted the audrow/add-package-export branch November 21, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants