Skip to content

[Merged by Bors] - feat: add slippery seaweed#641

Closed
Zac8668 wants to merge 4 commits intofishfolk:mainfrom
Zac8668:seaweed
Closed

[Merged by Bors] - feat: add slippery seaweed#641
Zac8668 wants to merge 4 commits intofishfolk:mainfrom
Zac8668:seaweed

Conversation

@Zac8668
Copy link
Collaborator

@Zac8668 Zac8668 commented Feb 22, 2023

No description provided.

@Zac8668 Zac8668 changed the title Add slippery seaweed feat: Add slippery seaweed Feb 22, 2023
@Zac8668 Zac8668 changed the title feat: Add slippery seaweed feat: add slippery seaweed Feb 22, 2023
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.

Looking good, thanks!

Just got a couple of things to tweak.

collision_world: CollisionWorld,
mut player_states: CompMut<PlayerState>,
) {
for (seaweed_ent, _) in entities.iter_with(&slippery_seaweeds) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this code to the slippery_seaweed.rs module?

That way if we add new items or things that can send the player into the incapacitated state, they can each be in their own module, and we don't have to keep making this system bigger to account for each new item.

add_state_module!(session, midair);
add_state_module!(session, walk);
add_state_module!(session, dead);
add_state_module!(session, incapacitaded);
Copy link
Member

@zicklag zicklag Feb 24, 2023

Choose a reason for hiding this comment

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

Since we don't need a player_state_transition system in the incapacitated state, don't use the module here and just register the state handler system:

Suggested change
add_state_module!(session, incapacitaded);
session
.stages
.add_system_to_stage(CoreStage::PreUpdate, incapacitated::handle_player_state);

@Zac8668 Zac8668 requested a review from zicklag March 1, 2023 06:23
Comment on lines +53 to +54
.add_system_to_stage(CoreStage::PreUpdate, incapacitated::handle_player_state)
.add_system_to_stage(PlayerStateStage, slippery_seaweed::update);
Copy link
Member

Choose a reason for hiding this comment

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

Tiniest little change: it'd be good to move the registration of the slippery seaweed system to it's own install function.

That should be the last thing!

Suggested change
.add_system_to_stage(CoreStage::PreUpdate, incapacitated::handle_player_state)
.add_system_to_stage(PlayerStateStage, slippery_seaweed::update);
.add_system_to_stage(CoreStage::PreUpdate, incapacitated::handle_player_state);

Copy link
Collaborator Author

@Zac8668 Zac8668 Mar 1, 2023

Choose a reason for hiding this comment

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

I tried to do this first actually, as it makes more sense, but I get this error:
thread 'main' panicked at 'Stage with label 'PlayerStateStage' ( 01GP72HD5B1S7VDNE9B2SAGWVX ) doesn't exist.'
Looks like this install function runs before the PlayerStateStage is created, not sure how to get around this.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. It must be because we haven't installed the player stage yet, when we install the elements.

You should be able to fix it by re-ordering these two lines here, so that the player systems are installed first:

jumpy/core/src/lib.rs

Lines 53 to 54 in dfba51a

elements::install(session);
player::install(session);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It worked now :)
(not sure how often you check discord, but I've asked you something on the jumpy channel, as you're awnsering fast here, just letting you know)

pub fn install(session: &mut GameSession) {
session
.stages
.add_system_to_stage(CoreStage::PreUpdate, hydrate);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.add_system_to_stage(CoreStage::PreUpdate, hydrate);
.add_system_to_stage(CoreStage::PreUpdate, hydrate)
.add_system_to_stage(PlayerStateStage, update);

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.

Perfect, thanks!

bors merge

bors bot pushed a commit that referenced this pull request Mar 1, 2023
@bors
Copy link
Contributor

bors bot commented Mar 1, 2023

@bors bors bot changed the title feat: add slippery seaweed [Merged by Bors] - feat: add slippery seaweed Mar 1, 2023
@bors bors bot closed this Mar 1, 2023
@Zac8668 Zac8668 deleted the seaweed branch March 1, 2023 18:23
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.

2 participants