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

Implement Debug for Schedule, Executor and other subtypes #213

Merged
merged 2 commits into from
Dec 13, 2020

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Oct 29, 2020

This is an attempt to implement #198. I've followed the guidelines for the Debug trait relatively strictly, so it mostly shows the exact internal state of the various types. I could make some of it a bit more clean and easy to read, for example by grouping the values of Executor by index, or showing only the system names instead of the full SystemId, but I first wanted some feedback on whether such a deviation from standard Debug output would be ok.

I introduced an instance of unsafe in the SystemBox implementation. I don't know if that is sound, that should be verified by someone who knows better.

Perhaps implementing this using the Debug trait is the wrong approach here, I just went with that because the original issue described it that way.

Copy link
Collaborator

@TomGillen TomGillen left a comment

Choose a reason for hiding this comment

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

I am sorry it has taken me so long to get to these PRs. These changes look good - I have suggested a note explaining why that unsafe should be OK.

src/internals/systems/schedule.rs Outdated Show resolved Hide resolved
Co-authored-by: Thomas Gillen <thomas.gillen@googlemail.com>
@TomGillen TomGillen merged commit 724420a into amethyst:master Dec 13, 2020
@TomGillen TomGillen added the type: feature New feature or request label Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants