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

Save with extension #142

Merged
merged 9 commits into from
Aug 22, 2023
Merged

Conversation

audrow
Copy link
Contributor

@audrow audrow commented Jul 6, 2023

Currently, when one saves a world, no extension is added unless the user types it out. Then when one goes to load a world, the extension must match a small list of extensions for it to be loaded.

Rather than have everyone go through the code, I figured to add the extension, if the user didn't type it out, so that the file can then be loaded easily.

@luca-della-vedova
Copy link
Member

This is very nice and I actually ran into this a few times so thanks a lot!
I gave it a try and it seems this would do a naive "append the extension", the problem with that is that we use a .building.yaml extension for legacy projects so test.building.yaml is saved as test.building.site.ron.
Would it be possible to make sure the whole old extension is stripped and the project is saved as test.site.ron instead?

@audrow audrow force-pushed the audrow/save-with-extension branch from e30b4f1 to 76c1fe8 Compare July 7, 2023 19:41
Signed-off-by: Audrow Nash <[email protected]>
I don't think I modified this.

Signed-off-by: Audrow Nash <[email protected]>
@audrow
Copy link
Contributor Author

audrow commented Jul 7, 2023

Would it be possible to make sure the whole old extension is stripped and the project is saved as test.site.ron instead?

Yup, this should be working now 👍

rmf_site_editor/src/save.rs Outdated Show resolved Hide resolved
rmf_site_editor/src/shapes.rs Outdated Show resolved Hide resolved
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.

I noticed that because of the way save events are sent there is a bit of a disconnect between the name of the file that is saved and the DefaultFile component (to be honest it also exists without this PR but this makes it more noticeable). I would expect the behavior of saving to be similar to how a classic editor would be, so that once the project is saved the project's default file is set to the newly saved path. It currently will print the conversion warning every time a .building.yaml project is saved until the project is closed and the .site.ron file is opened.

I wonder if we should update the DefaultFile component of the Site that is being saved to reflect the latest file that it was saved to, so that every subsequent Ctrl-S / Save will save to the last file that was specified (or in this case autocalculated).
What do you think?

@audrow
Copy link
Contributor Author

audrow commented Jul 10, 2023

I wonder if we should update the DefaultFile component of the Site that is being saved to reflect the latest file that it was saved to, so that every subsequent Ctrl-S / Save will save to the last file that was specified (or in this case autocalculated).

That seems like a good idea to me.

@mxgrey
Copy link
Collaborator

mxgrey commented Aug 22, 2023

I've push some changes that address the DefaultFile concern, which is definitely important.

Changing DefaultFile led me to realize another problem: We need to change the paths for all local relative-path asset sources. So I made a change to account for that.

...which led me to uncover a panic hazard which can happen if the AssetSource of a child visual within a URDF is changed at the same time as the AssetSource of the URDF model itself. To deal with that I had to introduce a ModelTrashcan that lets us defer deleting asset trees until asset modification commands have finished flushing.

Anyway I think this PR is ready to go now, after those unexpected complications 👍

@mxgrey mxgrey merged commit 696fd3c into open-rmf:main Aug 22, 2023
5 checks passed
@audrow audrow deleted the audrow/save-with-extension branch August 22, 2023 16:27
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.

3 participants