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

WIP: Save json with uri hint #1383

Open
wants to merge 3 commits into
base: production
Choose a base branch
from
Open

WIP: Save json with uri hint #1383

wants to merge 3 commits into from

Conversation

snozawa
Copy link
Collaborator

@snozawa snozawa commented May 3, 2024

Summary

  • Use uriHint by default.

Description

  • Without uriHint, referenceUri is contained into saved json file.
  • It seems the current jsonwriter.cpp only contains all the resolved bodies into one file, so referenceUri seems unnecessary.
  • In addition, if saved referenceUri is unnecessarily contained in the file,
    • When we load it by jsonreader.cpp, warning message is shown.
    • Other non-openrave model loader software cannot load it because referenced file does not exist.
  • I added noUriHint option just in case. It's unnecessary right now, but in future, once we support file saving preserving the reference files, it might be necessary.

Shunichi Nozawa added 2 commits May 3, 2024 20:53
- Without uriHint, referenceUri is contained into saved json file.
- It seems the current jsonwriter only contains all the resolved bodies into one file, so referenceUri seems unnecessary.
- In addition, if saved referenceUri is unnecessarily contained,
   - When we load it on openrave, warning message is shown.
   - Other model loader software cannot load it because referenced file does not exist.
- I added noUriHint option. It's unnecessary right now, but in future, once we support file saving preserving the reference files, it might be necessary.
@snozawa snozawa requested a review from rdiankov May 3, 2024 12:06
@snozawa snozawa self-assigned this May 3, 2024
@rdiankov
Copy link
Owner

rdiankov commented May 3, 2024

Changing default option like this could affect a lot... we should have ziyan and yoshiki comment on this.

@snozawa
Copy link
Collaborator Author

snozawa commented May 3, 2024

Changing default option like this could affect a lot... we should have ziyan and yoshiki comment on this.

thanks for your comment!

@yoshikikanemoto @ziyan

  • I guess, the current jsonwriter.cpp does not require referenceUri. I'm not fully sure about the negative effect if we remove referenceUri. But, I'm not also sure there is positive effect if we have referenceUri.
  • In my understanding, referenceUriHint is something like comment, and no behavioral effect for the saved json file. Is this correct?

Thanks!

p.s. It looks some tests are failing and needs to investigate.

@snozawa snozawa changed the title Save json with uri hint WIP: Save json with uri hint May 4, 2024
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