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

Introduce SystemExt to support pausable systems #1146

Merged
merged 5 commits into from Nov 24, 2018

Conversation

Projects
None yet
5 participants
@udoprog
Copy link
Member

udoprog commented Nov 15, 2018

This is a work in progress for better support to pause systems, see #1147 for more context.

@udoprog udoprog force-pushed the udoprog:enabled-systems branch from 139239c to b66ca97 Nov 15, 2018

@torkleyy
Copy link
Member

torkleyy left a comment

Ah nice, I see what you meant by the enum now. Thanks for the PR!

///
/// let mut dispatcher = DispatcherBuilder::default()
/// .with(Example(42), "set_number", &[])
/// .with(Example(84).enabled_on(State::Example), "set_number_2", &[])

This comment has been minimized.

@torkleyy

torkleyy Nov 15, 2018

Member

Thinking about this, this only allows intersection, but no unions. I'm wondering if we can allow both.

This comment has been minimized.

@udoprog

udoprog Nov 15, 2018

Member

Interesting!

I messed around with two different approaches:

  • Store a collection to check if the enum is a member of that collection (e.g. HashSet<State>).
  • Store the value as-is right now, but implement more complex testing associated with the system (e.g. OneOf<HashSet<State>>).

I landed on the second approach, because it ended up being significantly cleaner. More generic parameters could be elided instead of explicitly specified.

Here it is:
https://github.com/udoprog/amethyst/blob/enabled-system-predicates/src/system_extra.rs#L182

This can also support stuff like function-based testing.

What do you think?

This comment has been minimized.

@torkleyy

torkleyy Nov 15, 2018

Member

Part of me likes the pattern, but at the same time I'm unsure if this is actually helpful. I think if we show in the docs how to do more complex things (with concrete types), it helps them more than a generic API. So my suggestion to go forward with this would be making this very simple, just a Pausable system without any values and to show in the docs and book how to do more complex checks.

This comment has been minimized.

@udoprog

udoprog Nov 15, 2018

Member

Sorry, could you clarify:

How would the Pausable concretely work?

With complex checks are you referring to users either implementing the testing manually inside the systems or building their own delegate systems (like EnabledOn)?

This comment has been minimized.

@torkleyy

torkleyy Nov 15, 2018

Member

I'd imagine that the Pausable system just always uses one specific resource, defined in our utils crate. So just a simple boolean check on a resource.

If somebody wants something more complex, I think it's easier to show them how to build such a wrapper system on their own rather than making a generic API that covers it.

I'd like to hear some more comments on this. My motivation is just that I think generics make things difficult for the unfamiliar and in this case it's very simple to do it yourself.

This comment has been minimized.

@udoprog

udoprog Nov 15, 2018

Member

So refactored a bit now. I initially missed the part where you mentioned a resource in the utils crate. So a follow up:

Somehow we need to reference a collection of systems, and with a generic resource it would be through something like a name (&'static str). What did you have in mind here?
I do prefer using an enum, since it's type-checked at compile time to be a legal, pre-defined value (see the doc example). But this also currently supports anything implementing Eq.

This comment has been minimized.

@udoprog

udoprog Nov 15, 2018

Member

Oh, I think I understand now, you were expecting this to be used only for pausing a single set of systems?

I envisioned this to be used with having a set of systems per state that should be running, not only for the pause functionality.

As an example, you have a set of systems running for:

  • The game menu (UI + Menu Inputs).
  • The main game (Game UI + Game Logic + Game Inputs).
  • The game is paused (Pause Inputs).

@udoprog udoprog force-pushed the udoprog:enabled-systems branch from b66ca97 to 3be4896 Nov 15, 2018

@udoprog udoprog changed the title Introduce SystemExtra to implement enabled_on() Introduce SystemExtra to support pausable systems Nov 15, 2018

@udoprog udoprog force-pushed the udoprog:enabled-systems branch from 3be4896 to f4c51ef Nov 15, 2018

@jojolepro
Copy link
Member

jojolepro left a comment

Wow this turned out to be a clean solution. I wasn't expecting that at all.

I think it would make more sense to have this inside of the amethyst_core crate. That way, it will be also usable by people only using the subcrates (some people might not want to use State, but might use this pausable system).

Thanks!

/// };
///
/// #[derive(PartialEq, Eq)]
/// enum State {

This comment has been minimized.

@jojolepro

jojolepro Nov 16, 2018

Member

the State name is confusing for pretty obvious reasons. Mind changing it? :)

This comment has been minimized.

@udoprog

udoprog Nov 16, 2018

Member

Not at all!

@torkleyy
Copy link
Member

torkleyy left a comment

I love this, thank you for putting so much thought into this!

use specs::prelude::{Read, System};

/// Extra functionality associated systems.
pub trait SystemExtra {

This comment has been minimized.

@torkleyy

torkleyy Nov 16, 2018

Member

Okay, this is the only thing that's debatable IMO:

  • should this be in core? What's the reasoning for it?
  • should we call it SystemExtra (which implies more functionality will go into this)?

This comment has been minimized.

@jojolepro

jojolepro Nov 16, 2018

Member

@torkleyy see my review for the move to amethyst_core.

I'm pretty sure that could even be moved to an external specs_pausable crate if they really wanted too. Its generic enough to be that reusable.

This comment has been minimized.

@udoprog

udoprog Nov 17, 2018

Member

should we call it SystemExtra (which implies more functionality will go into this)?

Having one trait per "extension" would be a bit of a pain. I don't really see a reason to split them up assuming System itself stays object-safe, and the extensions have no dependencies.

This comment has been minimized.

@torkleyy

torkleyy Nov 17, 2018

Member

I just don't think this is a core thing. I'll let a third party decide.

@udoprog

This comment has been minimized.

Copy link
Member

udoprog commented Nov 17, 2018

Feel free to bikeshed on naming!

I'm not a huge fan of SystemExtra nor Pausable, but I also don't have better suggestions.

@torkleyy torkleyy requested a review from amethyst/engine-devs Nov 17, 2018

@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Nov 17, 2018

@udoprog What about SystemExt? System extension

@udoprog udoprog force-pushed the udoprog:enabled-systems branch from 405a1c7 to 4b84464 Nov 20, 2018

@udoprog udoprog changed the title Introduce SystemExtra to support pausable systems Introduce SystemExt to support pausable systems Nov 20, 2018

@torkleyy torkleyy requested a review from jojolepro Nov 22, 2018

@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Nov 22, 2018

@udoprog I think all that is left is to fix the merge conflict and we can merge!

@LucioFranco
Copy link
Member

LucioFranco left a comment

I really like this, I mostly have some questions that are not related to merging this but mostly for future possibilities.

I am curious if we can drop the SystemExt and just implement new for Pausable<S, V>?

So instead of thinking about it as an extension to a system more of a middleware setup. So you can wrap your systems in another system that provides functionality on top of it.

/// Enabled,
/// }
///
/// impl Default for CurrentState {

This comment has been minimized.

@LucioFranco

LucioFranco Nov 22, 2018

Member

Is there a reason in the docs you implemented Default instead of just explicitly setting it here? I personally find this hard to read.

https://github.com/amethyst/amethyst/pull/1146/files#diff-91c05f9b01ebc877ec04ac0957a6920bR57

This comment has been minimized.

@udoprog

udoprog Nov 23, 2018

Member

Non-default resources require ReadExpect, and have the "Tried to fetch a resource" error when they can't be found which is rather cryptic:

Tried to fetch a resource, but the resource does not exist.
Try adding the resource by inserting it manually or using the `setup` method.
You can get the type name of the missing resource by enabling `shred`'s `nightly` feature

This has generally tipped me more in favor of using resources which Default, since it guards against this kind of error through the type system.

@azriel91
Copy link
Member

azriel91 left a comment

I like this! Just the one question

fn pausable<V: 'static>(self, value: V) -> Pausable<Self, V>
where
Self: Sized,
V: Send + Sync + Default + Eq,

This comment has been minimized.

@azriel91

azriel91 Nov 22, 2018

Member

Can we relax the Eq to PartialEq? In case the resource used to determine pausing is PartialEq only, e.g. an enum with a variant that contains f32

This comment has been minimized.

@udoprog

@udoprog udoprog force-pushed the udoprog:enabled-systems branch from 4b84464 to 40a100e Nov 23, 2018

@torkleyy
Copy link
Member

torkleyy left a comment

This was open long enough now, let's merge it!
bors r=azriel91,jojolepro,torkleyy

bors bot added a commit that referenced this pull request Nov 24, 2018

Merge #1146
1146: Introduce SystemExt to support pausable systems r=azriel91,jojolepro,torkleyy a=udoprog

This is a work in progress for better support to pause systems, see #1147 for more context.

Co-authored-by: John-John Tedro <udoprog@tedro.se>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Nov 24, 2018

@bors bors bot merged commit 40a100e into amethyst:master Nov 24, 2018

4 checks passed

bors Build succeeded
Details
concourse-ci/linux-book-tests Concourse CI build success
Details
concourse-ci/linux-tests Concourse CI build success
Details
concourse-ci/rustfmt Concourse CI build success
Details

bors bot added a commit that referenced this pull request Nov 24, 2018

Merge #1154
1154: book: how to interact between states and systems using resources r=Moxinilian,azriel91,torkleyy a=udoprog

I promised folks during an Amethyst presentation I did in Gothenburg to improve documentation around the pain points I encountered while building [a test game](https://github.com/udoprog/asteroids-amethyst) in Amethyst.

This adds concrete documentation for how to actually implement the state transitions hinted at during the `State` section

I'm also actively working on trying to improve the ergonomics around this approach, so far the only relevant issue is: #1146 - but in general I'm trying to create a stronger association between a group of states and systems. If this becomes relevant, I'll update the documentation to reflect it :).

The other approach I'm aware of to handle this would be to set up event channels. But that section doesn't really talk about how to set up readers in states (it probably should!). Event channels is however a bit more complicates since the reader needs to be setup.

## Future TODOs?

Add concrete documentation for input handling - I mention adding an action called `open_menu`, but there's no great page to link to that discusses input handling generically, apart from [`pong-tutorial-03.md`](https://github.com/amethyst/amethyst/blob/master/book/src/pong-tutorial/pong-tutorial-03.md).
It would be great if we had a canonical section that dealt with only input handling in particular!

Co-authored-by: John-John Tedro <udoprog@tedro.se>
Co-authored-by: Azriel Hoh <azriel91@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment