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

--[BUGFIX] -Ellipses in paths #2218

Merged
merged 5 commits into from
Sep 27, 2023
Merged

--[BUGFIX] -Ellipses in paths #2218

merged 5 commits into from
Sep 27, 2023

Conversation

jturner65
Copy link
Contributor

@jturner65 jturner65 commented Sep 27, 2023

Motivation and Context

If an ellipsis is specified in a path and this ellipsis attempts to back-step across an OS link, functions that attempt to read this path will get lost. On such function we use is glob(). Such path configurations may be present in our scene dataset configurations, where the user might wish to access assets from another installed dataset that requires navigating to their directory outside the current dataset directory.

This PR adds a function that recursively removes ellipses if it can remove their parent directory (so a path with leading ellipses would be ignored). Tests are also added to verify functionality, and the path data being used for glob calls is now being appropriately filtered, albeit only in the case where the path within the scene dataset config path list starts with leading ellipses (in order to access assets or configs in another dataset, for instance).

For a future TODO I want to normalize all the metadata paths in the system, but that is outside the scope of what I need to do for Habitat 3.0 right now.

How Has This Been Tested

C++ and python tests pass locally (c++ test cases have been added to test the functionality)

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

If an OS link spans the ellipsis, certain functions, like glob, get lost when attempting to navigate the ellispis.
--glob doesn't know how to back-step across an os link, so it gets lost if it encounters one spanned by an ellipsis.
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 27, 2023
@0mdc
Copy link
Contributor

0mdc commented Sep 27, 2023

Nice! This was also causing caching issues in replay rendering, as the same file would sometimes be instantiated from different relative paths.

In an effort to limit the impact of this change, it has been narrowed to only affect paths prefixed with an ellipse in the scene dataset config. Since the scene dataset config is expected to exist at the root of the dataset, and since the scene dataset may be accessed via a link, having an ellipses from the dataset root back-pedals across a link. Some functions, like glob(), don't handle this properly and get lost. This commit addresses only these cases.
@jturner65
Copy link
Contributor Author

jturner65 commented Sep 27, 2023

Nice! This was also causing caching issues in replay rendering, as the same file would sometimes be instantiated from different relative paths.

A more rigorous handling of such cases is on my todo - eventually I would like to make all the paths within the system be stored normalized, but that is a bit more than I can handle right now.

This PR only addresses the case when someone specifies a path within the scene dataset config path lists with leading ellipses (in order to access assets or configs in another dataset, for instance). Since often datasets are accessed via links, this backpedaling across an OS link was causing glob() to break, which I use to build lists of paths specified in the config (the config handles glob wildcards in path names).

Copy link
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

LGTM!

@jturner65 jturner65 merged commit 7ed84da into main Sep 27, 2023
1 check passed
@jturner65 jturner65 deleted the Bugfix_ellipsesInPaths branch September 27, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants