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

[WIP] Implement Potion Effects #430

Closed
wants to merge 9 commits into from
Closed

[WIP] Implement Potion Effects #430

wants to merge 9 commits into from

Conversation

walker27460
Copy link

@walker27460 walker27460 commented Jun 3, 2021

Implement Potion Effects

Status

  • Ready
  • Development
  • Hold

Description

Implemented Potion Effects.

Related issues

Checklist

  • Ran cargo fmt, cargo clippy, cargo build --release and cargo test and fixed any generated errors!
  • Removed unnecessary commented out code
  • Used specific traces (if you trace actions please specify the cause i.e. the player)

@oxkitsune
Copy link
Member

Maybe some utility methods for adding potion effects to players need to be added? Just like the particle spawning methods, cause you could just add the components to the player. But I think having a util method would be good.

@walker27460
Copy link
Author

I'm gonna make method to get effect on quill.
and also spawn particle.

@walker27460 walker27460 marked this pull request as draft June 4, 2021 14:30
@walker27460 walker27460 changed the title Implement Potion Effects [WIP] Implement Potion Effects Jun 4, 2021
Copy link
Contributor

@ambeeeeee ambeeeeee left a comment

Choose a reason for hiding this comment

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

Just a few things I see, I know its not done yet.

Going forward, I think the best plan of action would be some form of trait for doing potion work. It would likely extend Entity so we can do entity.add_effect::<SlownessEffect>(5, 727727727); and so on.

Additionally, don't forget to write the systems to manage effect state every tick.

Comment on lines +145 to +176
impl_effect!(Speed);
impl_effect!(Slowness);
impl_effect!(Haste);
impl_effect!(MiningFatigue);
impl_effect!(Strength);
impl_effect!(InstantHealth);
impl_effect!(InstantDamage);
impl_effect!(JumpBoost);
impl_effect!(Nausea);
impl_effect!(Regeneration);
impl_effect!(Resistance);
impl_effect!(FireResistance);
impl_effect!(WaterBreathing);
impl_effect!(Invisibility);
impl_effect!(Blindness);
impl_effect!(NightVision);
impl_effect!(Hunger);
impl_effect!(Weakness);
impl_effect!(Poison);
impl_effect!(WitherEffect);
impl_effect!(HealthBoost);
impl_effect!(Absorption);
impl_effect!(Saturation);
impl_effect!(Glowing);
impl_effect!(Levitation);
impl_effect!(Luck);
impl_effect!(BadLuck);
impl_effect!(SlowFalling);
impl_effect!(ConduitPower);
impl_effect!(DolphinsGrace);
impl_effect!(BadOmen);
impl_effect!(HeroOfTheVillage);
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these should have the Effect suffix.

Comment on lines 115 to 116
pub ambient: bool, // given from beacon or not.
pub icon: bool, // show effect icon.
Copy link
Contributor

Choose a reason for hiding this comment

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

Switch the comments to doc comments on the members, consider adding one for particle and duration.

Comment on lines +136 to +143
macro_rules! impl_effect {
($ident:ident) => {
#[derive(Serialize, Deserialize, Eq, PartialEq, Hash)]
pub struct $ident(BTreeSet<PotionApplication>);
impl $ident {}
bincode_component_impl!($ident);
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might have somewhere we try to consolidate macros? Not sure.

Comment on lines 110 to 116
#[derive(Copy, Clone, Debug, Hash, Serialize, PartialEq, Eq, Deserialize)]
pub struct PotionApplication {
pub amplifier: u8,
pub duration: u32,
pub particle: bool,
pub ambient: bool, // given from beacon or not.
pub icon: bool, // show effect icon.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably move effects into their own module.

Copy link
Author

Choose a reason for hiding this comment

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

the effect enum was originally in libcraft but I made a transition to components.

@ambeeeeee
Copy link
Contributor

Hi there, it appears your PR has been inactive for a while. Are you planning to continue it? Please let me know so I can either pick it up or wait for more progress.

@walker27460
Copy link
Author

yeah im try to continue but no progress

@Defman
Copy link
Member

Defman commented Sep 16, 2021

Bump

@DenisTok DenisTok mentioned this pull request Dec 15, 2021
6 tasks
@koskja koskja mentioned this pull request Jan 2, 2022
6 tasks
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.

4 participants