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 a few more properties to legacy navgraphs #201

Merged
merged 10 commits into from
Mar 6, 2024

Conversation

luca-della-vedova
Copy link
Member

New feature implementation

Implemented feature

While working on SDF export, I noticed that several properties are missing, this PR brings us "closer" to all the features adding doors, lifts and orientation constraints. It also updates the TODOs to bring them up to date with what is needed since we will need to revisit this section.

Implementation description

I split this into a standalone PR since the SDF export is already becoming a very large change and this is somewhat orthogonal so it should be easier to review (and probably it would be better to scrutinize this a bit closer, since mistakes in an exported SDF are a lot easier to spot than mistakes in an exported navgraph).
I'll only be able to do full end to end testing when this is integrated with the SDF branch and we can export a full demo world, so this is somewhat experimental code (and parts like the lift center calculation might use another pair of eyes)

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Member Author

I added two small commits d76f218 and 3e94974 that fix two minor issues I encountered with the office world that made it not work (multiple locations were created with the same <Unnamed> name, as well as lanes were missing).
With these changes and the export_sdf branch the office demo almost works.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Member Author

With b4d0644 now the navgraph contains lift properties for vertices and lanes that contain lift anchors. On end to end demos robots can now use lifts.

rmf_site_format/src/legacy/nav_graph.rs Outdated Show resolved Hide resolved
rmf_site_format/src/legacy/nav_graph.rs Outdated Show resolved Hide resolved
rmf_site_format/src/legacy/nav_graph.rs Outdated Show resolved Hide resolved
rmf_site_format/src/legacy/nav_graph.rs Show resolved Hide resolved
rmf_site_format/src/legacy/nav_graph.rs Outdated Show resolved Hide resolved
rmf_site_format/src/legacy/nav_graph.rs Outdated Show resolved Hide resolved
rmf_site_format/src/legacy/nav_graph.rs Outdated Show resolved Hide resolved
rmf_site_format/src/lift.rs Outdated Show resolved Hide resolved
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova
Copy link
Member Author

All feedback should be addressed here.
I admit not really understanding the translation_for_category logic, I tried to export a few legacy buildings (i.e. hotel) to a .site.ron format and noticed that the only categorized anchors I could find where under the General category.

For the site anchors I was mostly thinking of the use case of a lane being between a site and a level anchor, for example if a user uses a fiducial anchor as the endpoint of a lane, and the second endpoint sits on a level anchor. But anyway since we don't want to allow site anchors because of the level ambiguity I just removed the logic.

luca-della-vedova and others added 4 commits March 5, 2024 17:18
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Copy link
Collaborator

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

I think the best way to fully validate the PR will be with e2e tests that show the properties are being used as intended by the fleet adapters, and we'll need to merge this PR before we can reach those e2e tests, so we should go ahead with this and apply corrections as needed later.

@luca-della-vedova luca-della-vedova merged commit b63a9fe into main Mar 6, 2024
5 checks passed
@luca-della-vedova luca-della-vedova deleted the luca/export_navgraphs branch March 6, 2024 02:09
reuben-thomas pushed a commit to reuben-thomas/rmf_site that referenced this pull request Jun 17, 2024
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Co-authored-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Reuben Thomas <reubenthomas123@gmail.com>
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