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

HandleApplyEffect #538

Closed
HashimTheArab opened this issue Jul 7, 2022 · 5 comments
Closed

HandleApplyEffect #538

HashimTheArab opened this issue Jul 7, 2022 · 5 comments
Labels
feature New feature or request

Comments

@HashimTheArab
Copy link
Contributor

This would allow users to easily modify an effect without creating a new custom one for little changes

Something like HandleApplyEffect(effect *effect.Effect) or HandleApplyEffect(effect effect.Effect, level *int, particlesHidden *bool, duration *time.Duration)

@Sandertv
Copy link
Member

Sandertv commented Jul 7, 2022

I have some reservations over this. Is this really a change that is necessary? Could you give an example where this can do something you wouldn't already be able to do differently?

@Sandertv Sandertv added the feature New feature or request label Jul 7, 2022
@HashimTheArab
Copy link
Contributor Author

For vasar kitpvp there's a perk that boosts the players speed level by 1 (only when they have a speed effect on)

What we could do is have a custom speed effect that implements that logic but even then the only thing we can modify is the level that's used in SetSpeed, we can't modify the actual level of the effect

Also it's a lot less convenient having to keep track of the custom effect making sure we only ever use that and not the dragonfly speed effect

@Sandertv
Copy link
Member

Sandertv commented Jul 7, 2022

Why isn't it possible to just create a new effect instance?

@HashimTheArab
Copy link
Contributor Author

That would have to be in every place we apply a speed effect, we could have a user.ApplySpeed function and always use that instead of player.AddEffect to apply speed but that feels wrong and might cause import cycles

@Sandertv
Copy link
Member

Sandertv commented Jul 9, 2022

As discussed in the Discord, we chose not to add this because a function as proposed (user.ApplySpeed) would solve this. Adding a handler for this method opens a can of worms where it is no longer clear if all methods need a handler or not.

@Sandertv Sandertv closed this as completed Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants