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

Crash with simultaneous ProgressPlugins (on different state types) #20

Open
siler opened this issue Nov 22, 2022 · 4 comments
Open

Crash with simultaneous ProgressPlugins (on different state types) #20

siler opened this issue Nov 22, 2022 · 4 comments

Comments

@siler
Copy link

siler commented Nov 22, 2022

I have two separate state machines, Client and Network. Client has the states LoadingGame and InGame and Network has Connecting and Connected. Each of these pairs has a ProgressPlugin associated with it. The transition of Network::Connecting to Network::Connected is one of the items I want to track in the outer progress plugin (Client->InGame), but when the inner one (Network->Connected) completes it pops the ProgressCounter resource leaving Client without one (at least I think this is what is happening, I haven't verified the root cause).

This might be solvable by making ProgressCounter generic over S in ProgressPlugin.

@inodentry inodentry changed the title Crash w/two states using nested ProgressPlugins Crash with simultaneous ProgressPlugins (on different state types) Nov 22, 2022
@inodentry
Copy link
Collaborator

Hmm. That makes sense. It never crossed my mind to think about progress tracking on multiple state types in parallel.

Now that you brought that up, I can see why things might break. It's a use case I'd like to support. I'll look into it.

You are probably right that (at least) the single ProgressCounter resource is messing up things. There might also be other things.

@inodentry
Copy link
Collaborator

inodentry commented Dec 5, 2022

I am trying to implement ProgressCounter being generic over the state type.

One downside I am running into is that now this would introduce extra boilerplate when adding systems. .track() is no longer enough; you would have to specify the state type you want to track for: .track::<S>(). This is completely understandable and desirable in your use case, where you have multiple different state types and you want to track progress independently for them. You would have to specify, for each system, which state type it applies to / where it tracks its progress.

However, this increases complexity and boilerplate for all users, and most users don't need progress tracking on more than one state type…

I wonder if we could come up with some sort of design that can keep things simple for most users, while still allowing for use cases like yours?

@inodentry
Copy link
Collaborator

@TheRawMeatball has given me some ideas, I will experiment with it.

inodentry added a commit that referenced this issue Mar 9, 2023
inodentry added a commit that referenced this issue May 6, 2023
* fix assets tracking logic

* depend on bevy main

* Udate to latest Bevy main; prep for 0.8 (#16)

* Update to latest Bevy commit

* Manually derive StageLabel

* Do not pin the Bevy commit

* Cargo.toml: point back at non-forked iyes_loopless

Co-authored-by: Ida Iyes <[email protected]>

* loopless moved some things around

* v0.4: Bevy 0.8

* refactor things conditional on loopless into separate files

* implement "hidden progress"

* abstract progress return types with a trait

* add hidden progress to example

* add dummy systems, they might be useful to someone

* version bump to 0.5.0

* update to iyes_loopless 0.8

* version bump to 0.6.0

* bevy 0.9 compat

* Mark progress plugin as not unique (#18)

* Version bump to 0.7.1

* unique name for each plugin instance (#19)

* add note about issue #20

* Bump Bevy to main 0.11-dev

---------

Co-authored-by: Ida Iyes <[email protected]>
Co-authored-by: Niklas Eicker <[email protected]>
Co-authored-by: Ida Iyes <[email protected]>
Co-authored-by: François <[email protected]>
NiklasEi added a commit to NiklasEi/iyes_progress that referenced this issue Oct 21, 2023
* fix assets tracking logic

* depend on bevy main

* Udate to latest Bevy main; prep for 0.8 (IyesGames#16)

* Update to latest Bevy commit

* Manually derive StageLabel

* Do not pin the Bevy commit

* Cargo.toml: point back at non-forked iyes_loopless

Co-authored-by: Ida Iyes <[email protected]>

* loopless moved some things around

* v0.4: Bevy 0.8

* refactor things conditional on loopless into separate files

* implement "hidden progress"

* abstract progress return types with a trait

* add hidden progress to example

* add dummy systems, they might be useful to someone

* version bump to 0.5.0

* update to iyes_loopless 0.8

* version bump to 0.6.0

* bevy 0.9 compat

* Mark progress plugin as not unique (IyesGames#18)

* Version bump to 0.7.1

* unique name for each plugin instance (IyesGames#19)

* add note about issue IyesGames#20

* Bump Bevy to main 0.11-dev

---------

Co-authored-by: Ida Iyes <[email protected]>
Co-authored-by: Niklas Eicker <[email protected]>
Co-authored-by: Ida Iyes <[email protected]>
Co-authored-by: François <[email protected]>
inodentry added a commit that referenced this issue Feb 18, 2024
* Update to latest Bevy main 0.11 (#23)

* fix assets tracking logic

* depend on bevy main

* Udate to latest Bevy main; prep for 0.8 (#16)

* Update to latest Bevy commit

* Manually derive StageLabel

* Do not pin the Bevy commit

* Cargo.toml: point back at non-forked iyes_loopless

Co-authored-by: Ida Iyes <[email protected]>

* loopless moved some things around

* v0.4: Bevy 0.8

* refactor things conditional on loopless into separate files

* implement "hidden progress"

* abstract progress return types with a trait

* add hidden progress to example

* add dummy systems, they might be useful to someone

* version bump to 0.5.0

* update to iyes_loopless 0.8

* version bump to 0.6.0

* bevy 0.9 compat

* Mark progress plugin as not unique (#18)

* Version bump to 0.7.1

* unique name for each plugin instance (#19)

* add note about issue #20

* Bump Bevy to main 0.11-dev

---------

Co-authored-by: Ida Iyes <[email protected]>
Co-authored-by: Niklas Eicker <[email protected]>
Co-authored-by: Ida Iyes <[email protected]>
Co-authored-by: François <[email protected]>

* Update to Bevy main 0.13

* Bump version for Bevy 0.13

* Bump Bevy dependency to 0.13

---------

Co-authored-by: José Pazos Pérez <[email protected]>
Co-authored-by: Ida Iyes <[email protected]>
Co-authored-by: Ida Iyes <[email protected]>
Co-authored-by: François <[email protected]>
@BeastLe9enD
Copy link

BeastLe9enD commented Aug 11, 2024

@inodentry has there been progress on this? I would also like to use this with different state types, and I think it would also be helpful to make AssetsLoading generic over T, so that we can specify assets that should be tracked only for a specific state.

If the complexity thing is still a problem, would it maybe be an idea to have two types, one for a single state (most people need) and one if you need multiple state types?

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

No branches or pull requests

3 participants