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

[Merged by Bors] - Add writing of scene data to Scene example #5949

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Add writing of scene data to Scene example #5949

wants to merge 3 commits into from

Conversation

wanderrful
Copy link
Contributor

Objective

Alice says to make this PR: https://discord.com/channels/691052431525675048/745805740274614303/1018554340841107477

  • The "scene" example in the examples folder has a TODO comment about writing the serialized data to a file. This PR implements that.

Solution

The AssetIo trait in the AssetServer only supports reading data, not writing it. So, I used std::io::File for the implementation. This way, every time you run the example, it will mutate the file in-place.

I had thought about adding a UUID string to the example Component, so that every time you run the example, the file will be guaranteed to change (currently, it just writes the same numbers over and over). However, I didn't bother because it was beyond the scope of the TODO comment.

One thing to note is that the logic for serializing the scene into RON data has changed since the existing RON file was created, and so even though the data is the same, it's rendered in a different order for whatever reason.

I left the changed output to the example file, because it's presumably trivial. I can remove it and force-push if you don't want that included in here.

@alice-i-cecile alice-i-cecile added C-Examples An addition or correction to our examples A-Scenes Serialized ECS data stored on the disk labels Sep 11, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Thanks! IMO we should write the scene to a new file to avoid altering the loaded data (and gitgnore the generated file).

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 11, 2022
# Objective

Alice says to make this PR: https://discord.com/channels/691052431525675048/745805740274614303/1018554340841107477

- The "scene" example in the examples folder has a TODO comment about writing the serialized data to a file. This PR implements that.

## Solution

The `AssetIo` trait in the `AssetServer` only supports reading data, not writing it. So, I used `std::io::File` for the implementation. This way, every time you run the example, it will mutate the file in-place.  

I had thought about adding a UUID string to the example Component, so that every time you run the example, the file will be guaranteed to change (currently, it just writes the same numbers over and over). However, I didn't bother because it was beyond the scope of the TODO comment.

One thing to note is that the logic for serializing the scene into RON data has changed since the existing RON file was created, and so even though the data is the same, it's rendered in a different order for whatever reason.

I left the changed output to the example file, because it's presumably trivial.  I can remove it and force-push if you don't want that included in here.
@bors bors bot changed the title Add writing of scene data to Scene example [Merged by Bors] - Add writing of scene data to Scene example Sep 11, 2022
@bors bors bot closed this Sep 11, 2022
@mockersf
Copy link
Member

This is a bad idea, and we should really not show usage of a blocking api (file writing) in a system.

@wanderrful
Copy link
Contributor Author

@mockersf What alternative do you suggest for this use case?

@mockersf
Copy link
Member

ideally, the asset server should be able to write assets...

But for now, we can just wrap it in a task: #5952

bors bot pushed a commit that referenced this pull request Sep 12, 2022
# Objective

- Fix #5951 
- Improve on #5949 by not risking blocking a system

## Solution

- Wrap file writing in a task
nicopap pushed a commit to nicopap/bevy that referenced this pull request Sep 12, 2022
# Objective

Alice says to make this PR: https://discord.com/channels/691052431525675048/745805740274614303/1018554340841107477

- The "scene" example in the examples folder has a TODO comment about writing the serialized data to a file. This PR implements that.

## Solution

The `AssetIo` trait in the `AssetServer` only supports reading data, not writing it. So, I used `std::io::File` for the implementation. This way, every time you run the example, it will mutate the file in-place.  

I had thought about adding a UUID string to the example Component, so that every time you run the example, the file will be guaranteed to change (currently, it just writes the same numbers over and over). However, I didn't bother because it was beyond the scope of the TODO comment.

One thing to note is that the logic for serializing the scene into RON data has changed since the existing RON file was created, and so even though the data is the same, it's rendered in a different order for whatever reason.

I left the changed output to the example file, because it's presumably trivial.  I can remove it and force-push if you don't want that included in here.
nicopap pushed a commit to nicopap/bevy that referenced this pull request Sep 12, 2022
# Objective

- Fix bevyengine#5951 
- Improve on bevyengine#5949 by not risking blocking a system

## Solution

- Wrap file writing in a task
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Alice says to make this PR: https://discord.com/channels/691052431525675048/745805740274614303/1018554340841107477

- The "scene" example in the examples folder has a TODO comment about writing the serialized data to a file. This PR implements that.

## Solution

The `AssetIo` trait in the `AssetServer` only supports reading data, not writing it. So, I used `std::io::File` for the implementation. This way, every time you run the example, it will mutate the file in-place.  

I had thought about adding a UUID string to the example Component, so that every time you run the example, the file will be guaranteed to change (currently, it just writes the same numbers over and over). However, I didn't bother because it was beyond the scope of the TODO comment.

One thing to note is that the logic for serializing the scene into RON data has changed since the existing RON file was created, and so even though the data is the same, it's rendered in a different order for whatever reason.

I left the changed output to the example file, because it's presumably trivial.  I can remove it and force-push if you don't want that included in here.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Fix bevyengine#5951 
- Improve on bevyengine#5949 by not risking blocking a system

## Solution

- Wrap file writing in a task
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Alice says to make this PR: https://discord.com/channels/691052431525675048/745805740274614303/1018554340841107477

- The "scene" example in the examples folder has a TODO comment about writing the serialized data to a file. This PR implements that.

## Solution

The `AssetIo` trait in the `AssetServer` only supports reading data, not writing it. So, I used `std::io::File` for the implementation. This way, every time you run the example, it will mutate the file in-place.  

I had thought about adding a UUID string to the example Component, so that every time you run the example, the file will be guaranteed to change (currently, it just writes the same numbers over and over). However, I didn't bother because it was beyond the scope of the TODO comment.

One thing to note is that the logic for serializing the scene into RON data has changed since the existing RON file was created, and so even though the data is the same, it's rendered in a different order for whatever reason.

I left the changed output to the example file, because it's presumably trivial.  I can remove it and force-push if you don't want that included in here.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Fix bevyengine#5951 
- Improve on bevyengine#5949 by not risking blocking a system

## Solution

- Wrap file writing in a task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Examples An addition or correction to our examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants