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

Add a new blockBreakEffect method and deprecate World#playEffect #2110

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Owen1212055
Copy link

  • Fixes the break effect used for super pickaxe using legacy ids
  • Fixes block break effect not displaying on fabric/forge
  • Allows you to now queue block effects of a blockstate rather than blocktype.

Please give feedback on the deprecation of World#playEffect and the old World#queueBlockBreakEffect method.
I was also split between making a separate method for the block break effect or making some kind of generic playEffect(Vector3 pos, int effect, T data) method that would do a bunch of instanceof checks to correctly adapt the data.
If a generic play effect method is preferred rather than a specific playBreakBlockEffect method, let me know!

@me4502
Copy link
Member

me4502 commented Jun 5, 2022

My intention was for the replacement to this method to be generic, however I hadn't looked into it given how complex it seemed to be to implement now

I feel a bunch of instanceof checks would become extremely messy. How do other APIs implement this?

@Owen1212055
Copy link
Author

My intention was for the replacement to this method to be generic, however I hadn't looked into it given how complex it seemed to be to implement now

I feel a bunch of instanceof checks would become extremely messy. How do other APIs implement this?

Well, i'm really only aware of the bukkit implementation, as it doesn't seem that the other platforms expose this at all. But other platforms such as bukkit check the individual keys which use the data field and cast it accordingly. There aren't many effects that use the data field, but it would require having a new Effect class instead of the int identifier. That can certainly be done if it's preferred.
image

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