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

Combine scene serialization sections #6

Merged
merged 4 commits into from
Nov 8, 2022

Conversation

rparrett
Copy link

@rparrett rparrett commented Nov 8, 2022

Not sure if I went too far with this.

Let me know if you have a better idea for how to lay this out.

),
]
```
See [this diff](https://github.com/bevyengine/bevy/compare/v0.8.0...main#diff-12b7f105bbb2a716e818a2a7eb2c79d40bef29ae8cba45b536c79cfa16b83558) for an illustration.
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't that just a full diff of bevy since 0.8? I think I'd prefer keeping the examples that were already present.

Copy link
Author

Choose a reason for hiding this comment

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

The intention was to link specifically to the diff for assets/scenes/load_scene_example.scn.ron. But that seems broken now. Will look into that, or just recreate the diff in a gist or something.

Keeping all the existing examples seems slightly problematic to me because we would be showing

Bevy 0.8 -> A format that is not valid
A format that is not valid -> Another format that is not valid
A format that is not valid -> Bevy 0.9

And this doesn't seem useful to me, personally.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I didn't realize those formats were all invalid. In that case maybe just paste the diff directly instead of a link?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea. I'll try that out

Copy link
Author

Choose a reason for hiding this comment

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

I'm not a huge fan of this either. I'm a bit lost about a good way to convey this information, really.

It was a bit better off-site as a side-by-side diff, but still not great.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I didn't realize those formats were all invalid.

That's a bit of an assumption, but I'm reasonably sure it holds.

I am assuming that most people affected will be re-serializing their scenes and not hand-editing them though so I'm not sure how useful any of this is.

Copy link
Owner

Choose a reason for hiding this comment

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

That's a good point actually, I guess the migration guide here is mostly about telling people that their old scene file won't work, not to update it manually. Do we have ways to re-serialize scenes though or would that just mean manually recreating it?

Copy link
Owner

Choose a reason for hiding this comment

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

I think I still like keeping the example there. It shows what's going on pretty well.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, sorry, by paste the diff I just meant to do like other guides and have it in 2 separate code block, one with 0.8 and one with 0.9

Done. That looks a lot better as a visual aid at least.

Do we have ways to re-serialize scenes though or would that just mean manually recreating it?

Might be worth asking around #scenes-dev to see if there's actually some sort of migration strategy for people. But as far as I know, it would mean manually recreating scenes.

Copy link
Author

Choose a reason for hiding this comment

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

I guess users might be confused about that too, so it might be worth spelling it out in the guide if that's the case.

@IceSentry IceSentry merged commit c19322b into IceSentry:migration-guide-0.9 Nov 8, 2022
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