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

Move away from System::setup, migrate to latest Specs. #1780

Merged
merged 62 commits into from Aug 6, 2019

Conversation

@jojolepro
Copy link
Member

commented Jul 4, 2019

Description

Move avay from using the System::setup method, as described by #1493

PR Checklist

By placing an x in the boxes I certify that I have:

  • Updated the content of the book if this PR would make the book outdated.
  • Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
  • Added unit tests for new code added in this PR.
  • Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

  • Ran cargo +stable fmt --all
  • Ran cargo clippy --all
  • Ran cargo test --all --features "empty"
@azriel91

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

jojolepro added some commits Jul 5, 2019

@azriel91
Copy link
Member

left a comment

Thanks for this!
There's one discussion point, but likely deferred to some other PR.
Also, could you add an entry to CHANGELOG.md?
(breaking, we probably want migration notes as well)

amethyst_animation/src/skinning/systems.rs Outdated Show resolved Hide resolved
amethyst_input/src/system.rs Outdated Show resolved Hide resolved
amethyst_network/src/bundle.rs Outdated Show resolved Hide resolved

jojolepro and others added some commits Jul 8, 2019

Merge pull request #21 from distransient/syssetup
Begin transition to Specs 0.15
@Frizi

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

How am I supposed to configure systems at their creation time? As constructor invocation is moved into the dispatcher, i need to be able to skeak additional data into that constructor somehow. This is important for windowing changes that are about to happen. I believe that current form of new based system setup in new specs isn't really enough for our purposes.

@jojolepro

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

I'm not sure I understand. You can simply add more parameters in the new method, or add a builder pattern to create the system? @Frizi

@Frizi

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

@jojolepro You are right. I'm sorry for some reason i thought that fn new was now part of the trait. Should read more before expressing concerns.

@Friz64

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

(I love getting Notifications because someone accidentally mentioned me instead of Frizi :D)

@distransient

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

I always assume you two are like hacker twins basically

@jaynus
Copy link
Contributor

left a comment

  1. All of the examples are broken. They all have a fetch(res) when the variable was renamed world. A simple search and replace fixes. They are all also broken by the .clone() bug, which breaks all examples due to clone() calling Fetch and not the deref. This is another search and replace on self.dimensions = new_dimensions.map(|d| d.clone());

  2. Docs & doctests are wrong in places. These all need to be updated. Example being amethyst_test

  3. I know this is a shred/specs difference, but now System's still have setup() and RunNow still has setup - and we still USE the setup function in RunNow. This is confusing?

  4. I just merged this into my current working project and ported it. This really needs a transition guide. The specs changelogs are, frankly, really hard to find and not useful.

@azriel91
Copy link
Member

left a comment

woosh, so many files again. Some reverts to do

amethyst_ui/src/button/builder.rs Outdated Show resolved Hide resolved
amethyst_ui/src/button/mod.rs Outdated Show resolved Hide resolved
amethyst_ui/src/label.rs Outdated Show resolved Hide resolved
amethyst_ui/src/lib.rs Outdated Show resolved Hide resolved
amethyst_utils/src/fps_counter.rs Outdated Show resolved Hide resolved
book/src/concepts/event-channel.md Outdated Show resolved Hide resolved
book/src/concepts/system.md Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
Merge pull request #22 from distransient/syssetup
Fix a lot of everything else

@distransient distransient changed the title [WIP] Move away from System::setup. [WIP] Move away from System::setup, migrate to latest Specs. Jul 18, 2019

@azriel91 azriel91 force-pushed the jojolepro:syssetup branch from e4f8968 to 2cc51eb Aug 2, 2019

azriel91 added some commits Aug 2, 2019

@@ -16,9 +16,11 @@ appveyor = { repository = "amethyst/amethyst" }
travis-ci = { repository = "amethyst/amethyst" }

[dependencies]
heck = "0.3.1"

This comment has been minimized.

Copy link
@distransient
@distransient
Copy link
Member

left a comment

Would like to have SystemDesc blanket impl we discussed but TBH this PR is big enough as it is. 🚢 it

@distransient distransient changed the title [WIP] Move away from System::setup, migrate to latest Specs. Move away from System::setup, migrate to latest Specs. Aug 2, 2019

@azriel91 azriel91 requested a review from Frizi Aug 4, 2019

@azriel91

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

I have WIP to allow ReaderId<*Event> and ReaderId<ComponentEvent> to be automatically generated by the macro, but would rather have those reviewed independently in its own PR.

@@ -168,11 +180,23 @@ impl<'a, 'b> GameDataBuilder<'a, 'b> {
/// // It is legal to register a system with an empty name
/// .with(NopSystem, "", &[]);
/// ~~~
pub fn with<S>(mut self, system: S, name: &str, dependencies: &[&str]) -> Self
pub fn with<SD, S>(

This comment has been minimized.

Copy link
@jojolepro

jojolepro Aug 4, 2019

Author Member

I still don't understand how it will be possible to add systems not implementing SystemDesc without having to put them in a bundle. Did I miss something?

Here's how one of my dispatchers looks like, for reference:
image

In there, there's quite a few systems that are defined externally, in crates that have no dependency on amethyst. This PR would mean that I need to put either ALL systems in a single bundle, or make multiple bundles to add the systems that are not implementing SystemDesc. Did I get that right?

This comment has been minimized.

Copy link
@azriel91

azriel91 Aug 4, 2019

Member

in crates that have no dependency on amethyst

ah, this would make it inconvenient, but not impossible. For the crate that does depend on amethyst and those systems (i.e. the one that references GameData), you can implement SystemDesc on a separate struct in that crate.

Alternatives:

  • Since the systems appear to be constructible without parameters (I may be wrong), it wouldn't be difficult to write a macro to generate a SomethingSystemDesc -> SomethingSystem. It doesn't need to be a proc macro.
  • Crates that provide systems depend on amethyst.
  • Crates that provide systems depend on amethyst_core and optionally amethyst_derive for the proc macro.
  • We move the trait to specs, though it doesn't really appear to be a specs concern (lazy instantiation is only required because Amethyst inserts resources into the World).

edit: Oops, forgot to confirm that yeap, the bundle methods you mentioned also work.

This comment has been minimized.

Copy link
@azriel91

azriel91 Aug 4, 2019

Member

Would like to mention, updating the crates that don't depend on amethyst (but do depend on specs) to depend on it would not be such a bad thing -- amethyst is compiled for the project anyway, and if they do depend on amethyst, the specs version is always kept consistent.

The con would be, if you build a crate via cargo build -p <crate>, then it would compile amethyst separately for that crate instead of specs downwards.

This comment has been minimized.

Copy link
@azriel91

azriel91 Aug 4, 2019

Member

ha, alternatively, we just expose another with_ method on GameData that takes a System directly 😄
why complicate things

note: the system will still be registered lazily, even though it's already constructed when passed in -- just need to add it to a DispatcherOperation. This is to ensure the system dependencies are available upon registration.

This comment has been minimized.

Copy link
@azriel91

azriel91 Aug 4, 2019

Member

Okay, implemented this, with takes a S: System as usual, with_system_desc takes the SD: SystemDesc

@jojolepro

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Allright... everything seems in order, so let's see if this big PR will merge ;)

bors r+

bors bot added a commit that referenced this pull request Aug 6, 2019

Merge #1780
1780: Move away from System::setup, migrate to latest Specs. r=jojolepro a=jojolepro

## Description

Move avay from using the System::setup method, as described by #1493

## PR Checklist

By placing an x in the boxes I certify that I have:

- [ ] Updated the content of the book if this PR would make the book outdated.
- [ ] Added a changelog entry if this will impact users, or modified more than 5 lines of Rust that wasn't a doc comment.
- [ ] Added unit tests for new code added in this PR.
- [ ] Acknowledged that by making this pull request I release this code under an MIT/Apache 2.0 dual licensing scheme.

If this modified or created any rs files:

- [ ] Ran `cargo +stable fmt --all`
- [ ] Ran `cargo clippy --all`
- [ ] Ran `cargo test --all --features "empty"`

Co-authored-by: Joël Lupien (Jojolepro) <jojolepromain@gmail.com>
Co-authored-by: Kel <kel@unclear.info>
Co-authored-by: Joël Lupien <jojolepromain@gmail.com>
Co-authored-by: Azriel Hoh <azriel91@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Build succeeded

@bors bors bot merged commit d3b7417 into amethyst:master Aug 6, 2019

2 checks passed

bors Build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@iamsauravsharma iamsauravsharma referenced this pull request Aug 7, 2019
3 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.