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 first version of camera stopping #171

Merged
merged 5 commits into from
Aug 2, 2022

Conversation

64kramsystem
Copy link
Member

@64kramsystem 64kramsystem commented Jul 28, 2022

Define points in the levels, where all the enemies (spawned) until then must be defeated before continuing.

The camera logic needs refinements (extra complexity) because for example, as of now, when there is a stop point, the players can't cross it, leaving a small part of the screen unusable by them.

Includes a refactoring: created the new bundle ActiveFighterBundle, and extracted fighter activation into a method of this type.

Closes #97 (will refine in a separate issue).

Depends on #99.

@64kramsystem 64kramsystem added this to the v0.0.4 milestone Jul 28, 2022
@64kramsystem 64kramsystem requested review from odecay and zicklag July 28, 2022 21:14
@64kramsystem 64kramsystem self-assigned this Jul 28, 2022
Copy link
Member

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

Looks good!

@zicklag
Copy link
Member

zicklag commented Jul 29, 2022

Do I remember correctly that we are still wanting to do a refactor that would allow us to clamp player positions in one place as opposed to using a helper function everywhere?

@64kramsystem
Copy link
Member Author

Do I remember correctly that we are still wanting to do a refactor that would allow us to clamp player positions in one place as opposed to using a helper function everywhere?

Yes! That's the plan. Once the stages are in place, and the movement are converted to a messaging system (intention of movement), the helper routine will become a system, and any place where the helper is currently invoked, a component (or message) will be instantiated 😄

@zicklag
Copy link
Member

zicklag commented Jul 29, 2022

OK cool, I was just making sure it didn't make sense to put the helper function for clamping the movement in a system parameter so it could be reliably used in multiple systems.

@64kramsystem
Copy link
Member Author

OK cool, I was just making sure it didn't make sense to put the helper function for clamping the movement in a system parameter so it could be reliably used in multiple systems.

Oh, sure. How would that work? A sort of service resource? I don't think I've ever done this 😄

All of this PR content is going away very soon, however, I'm definitely interested in (Bevy/ECS) design patterns, and it doesn't hurt to have cleaner intermediate code 😄

@zicklag
Copy link
Member

zicklag commented Jul 29, 2022

Well I'm still relatively new to actually using an ECS, too, so I'm not always sure when it's best to use it or not use, but it works sort of like a "service resource".

It lets use wrap up any number of queries or resources into a single system argument that you can add helper methods to.

Doing it like this has a benefit of not having to manually include all the resources and queries that you need to do clamping in your system.

// Usually you just name the 'world and 'state lifetimes 'w and 's, but I named them here
// just to be more descriptive. They represent the fact that we are borrowing some data from
// the ECS world, and some data from our system's own state. ( as far as I understand )
#[derive(SystemParam)]
struct PlayerMovementClamper<'world, 'state> {
    enemy_spawn_locations: Query<'world, 'state, &SpawnLocationX>,
    level_meta: Res<'world, LevelMeta>,
    game_meta: Res<'world, GameMeta>,
    left_movement_boundary: Res<'world, LeftMovementBoundary>,
}

impl<'w, 's> PlayerMovementClamper<'w, 's> {
    pub fn clamp_movements(
        &self, // Self will have all of our queries we need for clamping
        mut player_movements: Vec<(Vec3, Option<Vec2>)>,
    ) -> Vec<Option<Vec2>> {
        // Impl clamping 
    }
}

/// Then we can use it in a system like this:
fn my_system(clamper: PlayerMovementClamper, /* Whatever other args I need for my system */) {
    // Some player movements
    let player_movements = todo!();

    let clamped_player_movements = clamper.clamp_movements(player_movements);
}

The caveat is that if the queries in the system param conflict with queries you need in that system you have to avoid the conflict in one of two ways:

  1. Use a ParamSet to split the borrow of the system parameters safely.
  2. Construct the PlayerMovementClamer inside your system ( I think that works, but I haven't tried it ) like this:
fn my_system(
    enemy_spawn_locations: Query<&SpawnLocationX>,
    level_meta: Res<LevelMeta>,
    game_meta: Res<GameMeta>,
    left_movement_boundary: Res<LeftMovementBoundary>
) {
    let clamped_player_movements = PlayerMovementClamper {
            enemy_spawn_locations,
            level_meta,
            game_meta,
            left_movement_boundary,
        }
        .clamp_movement(player_movements);
}

@zicklag
Copy link
Member

zicklag commented Jul 29, 2022

Also "reliably" was the wrong word above. I had misread and thought that you could enforce that the movement of all players would be passed in if you had control over the queries, but I was wrong. The system param would really just be for the convenience.

@64kramsystem
Copy link
Member Author

Interesting! Yes, this is what I was imagining with service resource (although, this Bevy pattern is nicer 😄).

Since this would require changing the system signature, for it to be changed back after the states PR, I'm going to put this PR on hold, then I'm going to rebase it, so we can skip directly to the final version.

The next task I'm going to do is the enemy FOV (which is very small), then I'll jump into the stages PR.

@64kramsystem 64kramsystem marked this pull request as draft July 29, 2022 21:59
@64kramsystem 64kramsystem marked this pull request as ready for review August 2, 2022 20:25
@64kramsystem
Copy link
Member Author

Ok, updated to use SystemParam, so that this can get out of the way for 0.0.4 😄

Copy link
Member

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

Awesome, just had one question.

@@ -68,6 +68,10 @@ enemies:
- fighter: *brute
location: [450, 20, 0]
trip_point_x: 300
- fighter: *brute
location: [1000, 20, 0]
Copy link
Member

Choose a reason for hiding this comment

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

Does this brute need a trip point specified?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! It got lost in a rebase conflict 😅

Copy link
Member

Choose a reason for hiding this comment

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

I've been there 😄

I've done a lot of ugly rebases in the past.

Define points in the levels, where all the enemies (spawned) until then must be defeated before continuing.

The camera logic needs refinements (extra complexity) because for example, as of now, when there is a stop point, the players can't cross it, leaving a small part of the screen unusable by them.
Still not a good design, but SystemParam is much better than a helper system.
Copy link
Member

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

Just tested it out, works great! 👍

bors r+

@zicklag
Copy link
Member

zicklag commented Aug 2, 2022

Also, just curious, was the FOV work supposed to replace the trip point concept?

@bors
Copy link
Contributor

bors bot commented Aug 2, 2022

Build succeeded:

@bors bors bot merged commit 72c83a6 into fishfolk:master Aug 2, 2022
@64kramsystem
Copy link
Member Author

Also, just curious, was the FOV work supposed to replace the trip point concept?

The opposite! This is the improved version. FOV was something temporary, meant to avoid all the enemies of a level rushing to the player. Trip points can do the same, but they're more flexible (for example, to design waves with certain shapes etc.).

@64kramsystem 64kramsystem deleted the camera_stop_points branch August 2, 2022 20:51
@zicklag
Copy link
Member

zicklag commented Aug 2, 2022

Got it, makes sense. 👍

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.

Implement camera stop points
2 participants