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 wrapper for BossBar packet #990

Merged
merged 8 commits into from
Sep 20, 2024
Merged

Add wrapper for BossBar packet #990

merged 8 commits into from
Sep 20, 2024

Conversation

Elikill58
Copy link
Contributor

@Elikill58 Elikill58 commented Sep 15, 2024

This PR add a wrapper for BOSS_BAR packet.

I pre-answer few questions:

  • I made my own Color enum to prevent switch/case to read color. It also help to other developer to only use right colors
  • The constructor isn't with all parameters as they are different between each actions
  • For the flags, I didn't convert them to something else that byte as it seems to changed multiple times. Also, I'm reading as UnsignedByte but there isn't writeUnsignedByte, I tried writeShort but that was too much.
  • In general, I used description available here and tested on my own server

I suggest you to Squash and merge. That's would be perfect.

@booky10
Copy link
Collaborator

booky10 commented Sep 15, 2024

Using adventure's bossbar classes would be a lot nicer to work with

@Elikill58
Copy link
Contributor Author

Elikill58 commented Sep 15, 2024

Using adventure's bossbar classes would be a lot nicer to work with

That's can be difficult as only one action has informations about the full bossbar, else it's only a part of it. Should I add something that is a partial bossbar, return null when not full or do nothing?

@booky10
Copy link
Collaborator

booky10 commented Sep 16, 2024

I meant using Bossbar.Color, etc.
Not constructing adventure bossbar objects

@Elikill58
Copy link
Contributor Author

Oh yes, it's good idea. I just made changes about this :)

@booky10
Copy link
Collaborator

booky10 commented Sep 16, 2024

could you also parse the flags sent in the packet?

@Elikill58
Copy link
Contributor Author

I'll check. It will require more testing as it's more versions depending

@Elikill58
Copy link
Contributor Author

That's it! :) (Note: please don't forget to squash, it's better)

Copy link
Collaborator

@booky10 booky10 left a comment

Choose a reason for hiding this comment

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

one small issue left, the rest looks good 👍

Copy link
Collaborator

@booky10 booky10 left a comment

Choose a reason for hiding this comment

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

👍

@retrooper
Copy link
Owner

Thanks for your PR.

@retrooper retrooper merged commit 243ce3c into retrooper:2.0 Sep 20, 2024
2 checks passed
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.

3 participants